Should we hold the softc write lock when setting sc->so?

This can be one line.

I suspect it'd be a bit better to set the tunneling function after finding a slot in the peer table. If we fail to find a slot, we leave the tunneling function set.

The callout requires the sc->lock, so I wouldn't expect that to be an issue.

I'm also seeing panics over sleepq_add with sleeping prohibited if we sleep here. Thinking about it some more I'm wondering if the drain for ping_send shouldn't also be a stop. It takes the same lock, so it should be similarly safe.

I don't think this is guaranteed to be contiguous, but I also don't think it matters. m_copyback() deals with mbuf chains and ovpn_encap() basically just prepends data.

Good catch.

The first potential fix that comes to mind is to not reset the callout, but to track the timestamp of the last packet (with second level accuracy). We can then perform the check in the ovpn_timeout_peer() callback and either restart the callout or handle a timeout.

It may be worth replicating the trick Reid used in pf (https://cgit.freebsd.org/src/commit/sys?id=0abcc1d2d33aef2333dab28e1ec1858cf45b314a), where we have a per-cpu variable to write to. If we go that route it might be worth abstracting that a little.

I think you're right that the ping_rcv callout is ok, my mistake. But the ping_snd callout handler drops the softc lock while executing, and we really do need to defer the free of peer until that's done.

I think this comment dates back to the initial revision of the patch, which had

m_defrag(m);
nvlist_add_binary(... mtod(m, uint8_t *), m->m_pkthdr.len);

Now it's ok because we allocate a contiguous buffer, copy the packet, and move it into the nvlist.

Hmm. Do you have any ideas on how to cope with that?

We're kind of stuck passing at least the softc, both for counters and in some cases because we need to look up the peer (in ovpn_encap(), from ovpn_encrypt_tx_cb()).

Yes. The protocol doesn't really have a mechanism to cope with that, but in practice it's not much of a concern because it will negotiate a new key and switch to it (which resets the sequence counter) long before we've passed enough packets for that.

I don't think it does. Not any more at least.

It keeps the lock, because it's a read lock and the callout support for rmlocks is a little limited, in that it doesn't export the rm_priotracker anywhere, so there's no way for callout users to release an rm-read lock (unlike mutex, or rwlock-based callouts).

Ah, got it. I missed that the tracker pointer is NULL in that case.

An atomic reference count is the standard way to handle it. That's how IPSec solves this problem.

If you ignore the possibility of hardware offloads, then I believe it would be safe to simply hold the softc lock across the crypto_dispatch() call. The ping_send callout does this. I don't believe any of our hw offload drivers implement CHACHA20, for what it's worth, though that of course could change.

If you want to be able to make use of hardware offloads and need to avoid the overhead of a global refcount, then per-CPU reference counting for each peer would probably be the way to go. I don't believe we have a generic implementation of that, though. I would probably try adding per-peer atomic refcounting and see how affects throughput, since that's easy to implement and think about, and can be replaced with something more sophisticated later.

Is it guaranteed that there are no IPv6 extension options following the fixed-length header?

bump the year?

These are identical to DIOCSTART / DIOCSTOP. I don't think there's an opportunity for confusion, but do we have some existing guidance or reference on IOCTL assignments?

None that I'm aware of.

Yes, because we're creating the IPv6 header ourselves here.

This is a stack declaration. Please move it above M_ASSERTPKTHDR() which is a statement.

Can we please count differently packets lost due to crypto failures and due to memory allocation failures? Just add one or two more counters.

Why not using refcount(9)?

If counters were split out into a structure, could be initialized with one liner macros like ipstat, tcpstat. Exporting them to userland as a structure also seems easier.

Does it have to be present when the kernel gets control, or is loading it during multiuser startup (rc) early enough?

It can be loaded at any time prior to its use. The upcoming openvpn version with DCO support should actually attempt to load it automatically if you attempt to use DCO on FreeBSD.

Then I'd mention the autoloading thing and use rc.conf loading instead (with or without bringing in sysrc(8), not sure).

This is the standard module text, I'm not sure we should be deviating from that.

See for example man 4 if_bridge or man 4 if_epair.

Q: what about link-local addresses?

It should be peer ( or process) fib, not 0.

I'm wondering if we can build this prepend once for a peer and then use it via simply memcpy() (maybe with a bit of tiny per-af tweaks).

Probably worth having struct route in the peer data & reference it here, so we leverage L3+L2 cache lookups.

Interesting question, I didn't think about them.

I've done a little digging in the OpenVPN code, and it appears that OpenVPN also doesn't support configuration of link-local addresses, so right now I'd be inclined to not support them here.

The ioctl interface uses nvlists, so if that changes in the future it'll be relatively easy to add this support. Although to be honest, I think the use case for a VPN over link-local addresses is fairly weak.

Interesting idea, but there are some cache invalidation issues.

We'd have to be careful about rebuilding it if the peer configuration changes. That's manageable, but we also use V_ip_defttl, so if we cached it we'd end up not changing TTL when the default is changed. Pretty minor of course, but I'm also skeptical that there's a measurable performance improvement in doing this.

We'd also still have to tweak things a bit for length and checksum fields, for every packet.

Ooh, interesting.

Although I worry about locking of the 'struct route'. if_ovpn uses an rmlock and multiple cores may be running ip_output() at the same time. It's also unpredictable if ip_output() will modify the 'struct route', because the routing table could have been updated. (so if_ovpn can't just take the write lock when it has an invalid struct route).

I wonder if we should keep a per-CPU 'struct route'.

Point. (I think that decision should be revisited, but this is not the place for that discussion.) I'd still mention the autoloading thing, though.