Date: Fri, 06 Sep 2024 18:49:48 +0200 From: Kristof Provost <kp@FreeBSD.org> To: Steve Kargl <sgk@troutmask.apl.washington.edu> Cc: freebsd-hackers@freebsd.org Subject: Re: New lock-order reversal Message-ID: <72AF10E0-CB5A-4CB6-A50A-30A06DB7EA03@FreeBSD.org> In-Reply-To: <ZtslrWXXaSL74v0A@troutmask.apl.washington.edu> References: <ZtslrWXXaSL74v0A@troutmask.apl.washington.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
--=_MailMate_9C6D674F-29D9-450A-954C-B7353E708F39_=
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
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
--=_MailMate_9C6D674F-29D9-450A-954C-B7353E708F39_=
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
<!DOCTYPE html>
<html>
<head>
<meta http-equiv=3D"Content-Type" content=3D"text/xhtml; charset=3Dutf-8"=
>
</head>
<body><div style=3D"font-family: sans-serif;"><div class=3D"markdown" sty=
le=3D"white-space: normal;">
<p dir=3D"auto">Hi Steve,</p>
<p dir=3D"auto">On 6 Sep 2024, at 17:54, Steve Kargl wrote:</p>
</div><div class=3D"plaintext" style=3D"white-space: normal;"><blockquote=
style=3D"margin: 0 0 5px; padding-left: 5px; border-left: 2px solid #136=
BCE; color: #136BCE;"><p dir=3D"auto">FYI (and return hackers to a non-la=
nguage)</p>
<p dir=3D"auto">Update my old system to circa Aug 10, 2024 top-of-tree
<br>
and rebuilts all installed ports. I'm now see a new
<br>
lock-order reversal while using openvpn.</p>
<p dir=3D"auto">lock order reversal:
<br>
1st 0xfffff801e5482aa0 udpinp (udpinp, rw) @ /usr/src/sys/netinet/udp_us=
rreq.c:1129
<br>
2nd 0xfffff80003510188 if_ovpn_lock (if_ovpn_lock, rm) @ /usr/src/sys/ne=
t/if_ovpn.c:2118
<br>
lock order if_ovpn_lock -> udpinp established at:
<br>
#0 0xffffffff80ba66fa at witness_checkorder+0x32a
<br>
#1 0xffffffff80b2d692 at _rw_wlock_cookie+0x62
<br>
#2 0xffffffff80d45e4e at udp_set_kernel_tunneling+0x4e
<br>
#3 0xffffffff82a783a1 at ovpn_ioctl_set+0xe81
<br>
#4 0xffffffff82a76df8 at ovpn_ioctl+0xf8
<br>
#5 0xffffffff80c62179 at ifioctl+0x949
<br>
#6 0xffffffff80bacd26 at kern_ioctl+0x286
<br>
#7 0xffffffff80baca3d at sys_ioctl+0x12d
<br>
#8 0xffffffff8104b018 at amd64_syscall+0x158
<br>
#9 0xffffffff8101d73b at fast_syscall_common+0xf8
<br>
lock order udpinp -> if_ovpn_lock attempted at:
<br>
#0 0xffffffff80ba6f75 at witness_checkorder+0xba5
<br>
#1 0xffffffff80b2c22c at _rm_rlock_debug+0x12c
<br>
#2 0xffffffff82a76e91 at ovpn_output+0x41
<br>
#3 0xffffffff80d0e1ce at ip_output+0x150e
<br>
#4 0xffffffff80d45ad9 at udp_send+0xa69
<br>
#5 0xffffffff80be56d1 at sosend_dgram+0x311
<br>
#6 0xffffffff80be62b9 at sousrsend+0x79
<br>
#7 0xffffffff80bec35c at kern_sendit+0x1bc
<br>
#8 0xffffffff80bec66b at sendit+0x1ab
<br>
#9 0xffffffff80bec4ad at sys_sendto+0x4d
<br>
#10 0xffffffff8104b018 at amd64_syscall+0x158
<br>
#11 0xffffffff8101d73b at fast_syscall_common+0xf8</p>
<br></blockquote></div>
<div class=3D"markdown" style=3D"white-space: normal;">
<p dir=3D"auto">I don=E2=80=99t think that=E2=80=99s new. It=E2=80=99s 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 loc=
k.</p>
<p dir=3D"auto">I=E2=80=99ve had another look at this, and while I can pr=
obably avoid this for setting the tunnel function (basically by assuming =
setting it never fails or is already done, which is currently the case), =
I=E2=80=99m not happy with the only solution I see on the removal side (i=
=2Ee. =E2=80=9Cdon=E2=80=99t, just trust that the socket will be closed s=
oon=E2=80=9D).</p>
<pre style=3D"margin-left: 15px; margin-right: 15px; padding: 5px; border=
: thin solid gray; overflow-x: auto; max-width: 90vw; background-color: #=
E4E4E4;"><code style=3D"padding: 0 0.25em; background-color: #E4E4E4;">di=
ff --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_kpee=
r *peer)
has_peers =3D 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, NUL=
L);
-
sorele(sc->so);
if (! has_peers)
@@ -636,18 +631,10 @@ ovpn_new_peer(struct ifnet *ifp, const nvlist_t *nv=
l)
sc->peercount++;
soref(sc->so);
- ret =3D udp_set_kernel_tunneling(sc->so, ovpn_udp_input, NULL,=
sc);
- if (ret =3D=3D EBUSY) {
- /* Fine, another peer already set the input function. */
- ret =3D 0;
- }
- if (ret !=3D 0) {
- RB_REMOVE(ovpn_kpeers, &sc->peers, peer);
- sc->peercount--;
- goto error_locked;
- }
-
OVPN_WUNLOCK(sc);
+ ret =3D udp_set_kernel_tunneling(sc->so, ovpn_udp_input, NULL,=
sc);
+ MPASS(ret =3D=3D 0 || ret =3D=3D EBUSY);
+ ret =3D 0;
goto done;
</code></pre>
<p dir=3D"auto">Best regards,<br>
Kristof</p>
</div>
</div>
</body>
</html>
--=_MailMate_9C6D674F-29D9-450A-954C-B7353E708F39_=--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?72AF10E0-CB5A-4CB6-A50A-30A06DB7EA03>