Re: New lock-order reversal

Hi Steve,

On 6 Sep 2024, at 17:54, Steve Kargl wrote:
> FYI (and return hackers to a non-language)
>
> Update my old system to circa Aug 10, 2024 top-of-tree
> and rebuilts all installed ports.  I'm now see a new
> lock-order reversal while using openvpn.
>
> lock order reversal:
>  1st 0xfffff801e5482aa0 udpinp (udpinp, rw) @ 
> /usr/src/sys/netinet/udp_usrreq.c:1129
>  2nd 0xfffff80003510188 if_ovpn_lock (if_ovpn_lock, rm) @ 
> /usr/src/sys/net/if_ovpn.c:2118
> lock order if_ovpn_lock -> udpinp established at:
> #0 0xffffffff80ba66fa at witness_checkorder+0x32a
> #1 0xffffffff80b2d692 at _rw_wlock_cookie+0x62
> #2 0xffffffff80d45e4e at udp_set_kernel_tunneling+0x4e
> #3 0xffffffff82a783a1 at ovpn_ioctl_set+0xe81
> #4 0xffffffff82a76df8 at ovpn_ioctl+0xf8
> #5 0xffffffff80c62179 at ifioctl+0x949
> #6 0xffffffff80bacd26 at kern_ioctl+0x286
> #7 0xffffffff80baca3d at sys_ioctl+0x12d
> #8 0xffffffff8104b018 at amd64_syscall+0x158
> #9 0xffffffff8101d73b at fast_syscall_common+0xf8
> lock order udpinp -> if_ovpn_lock attempted at:
> #0 0xffffffff80ba6f75 at witness_checkorder+0xba5
> #1 0xffffffff80b2c22c at _rm_rlock_debug+0x12c
> #2 0xffffffff82a76e91 at ovpn_output+0x41
> #3 0xffffffff80d0e1ce at ip_output+0x150e
> #4 0xffffffff80d45ad9 at udp_send+0xa69
> #5 0xffffffff80be56d1 at sosend_dgram+0x311
> #6 0xffffffff80be62b9 at sousrsend+0x79
> #7 0xffffffff80bec35c at kern_sendit+0x1bc
> #8 0xffffffff80bec66b at sendit+0x1ab
> #9 0xffffffff80bec4ad at sys_sendto+0x4d
> #10 0xffffffff8104b018 at amd64_syscall+0x158
> #11 0xffffffff8101d73b at fast_syscall_common+0xf8
>
I don’t think that’s new. It’s an order issue between if_ovpn 
establishing the UDP tunnel callback (which requires the UDP lock) and 
the normal traffic flow, where the UDP lock is taken, the tunnel 
function is called and that then takes the if_ovpn lock.

I’ve had another look at this, and while I can probably avoid this for 
setting the tunnel function (basically by assuming setting it never 
fails or is already done, which is currently the case), I’m not happy 
with the only solution I see on the removal side (i.e. “don’t, just 
trust that the socket will be closed soon”).

	diff --git a/sys/net/if_ovpn.c b/sys/net/if_ovpn.c
	index ee097cfa24b3..322ac59e6dd0 100644
	--- a/sys/net/if_ovpn.c
	+++ b/sys/net/if_ovpn.c
	@@ -390,11 +390,6 @@ ovpn_rele_so(struct ovpn_softc *sc, struct 
ovpn_kpeer *peer)

	        has_peers = ovpn_has_peers(sc);

	-       /* Only remove the tunnel function if we're releasing the 
socket for
	-        * the last peer. */
	-       if (! has_peers)
	-               (void)udp_set_kernel_tunneling(sc->so, NULL, NULL, 
NULL);
	-
	        sorele(sc->so);

	        if (! has_peers)
	@@ -636,18 +631,10 @@ ovpn_new_peer(struct ifnet *ifp, const nvlist_t 
*nvl)
	        sc->peercount++;
	        soref(sc->so);

	-       ret = udp_set_kernel_tunneling(sc->so, ovpn_udp_input, NULL, 
sc);
	-       if (ret == EBUSY) {
	-               /* Fine, another peer already set the input function. 
*/
	-               ret = 0;
	-       }
	-       if (ret != 0) {
	-               RB_REMOVE(ovpn_kpeers, &sc->peers, peer);
	-               sc->peercount--;
	-               goto error_locked;
	-       }
	-
	        OVPN_WUNLOCK(sc);
	+       ret = udp_set_kernel_tunneling(sc->so, ovpn_udp_input, NULL, 
sc);
	+       MPASS(ret == 0 || ret == EBUSY);
	+       ret = 0;

	        goto done;

Best regards,
Kristof