From nobody Fri Jan 21 13:33:27 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 3112B19531CF; Fri, 21 Jan 2022 13:33:30 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4JgL1Q0dchz4vr7; Fri, 21 Jan 2022 13:33:30 +0000 (UTC) (envelope-from kp@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1642772010; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=V9UhT/7/Xm4SUoWqoLqXRXmho3pXxqlQOWGg/Escky8=; b=V4cbliux0wnwandK4bLNjekOSzuAkkRkv/wxia3vJG1psCGiW1fs32w0ef7XVXN8FD5rpU fHl0jfPPZtyevydUDyHzKGGerwYoGotxRNJgiCevfSv5fPGvbb5Zbo9I9RpbEUfv8K7ruK jGSqfUVWBKPXHHmke0fX0ZbJOAFHUXh+YgxkYCjQsrJb50Ms7WShYccrVOOyAeN67C6nwU 4QBjDpbYOSFZ1Gh2WeY4qu+3Ao01QdoFfTB9O/ahHB36EXae4J58jZ/qWmXWV/XxabFAPc 0T5k0EN//Vm+RUQeM3xgjKG5jepeae/NxsDJqv3a7uS4065mLuGbGA+EnRZZpw== Received: from venus.codepro.be (venus.codepro.be [5.9.86.228]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mx1.codepro.be", Issuer "R3" (verified OK)) (Authenticated sender: kp) by smtp.freebsd.org (Postfix) with ESMTPSA id BC46B2ABAF; Fri, 21 Jan 2022 13:33:29 +0000 (UTC) (envelope-from kp@FreeBSD.org) Received: by venus.codepro.be (Postfix, authenticated sender kp) id 1D89A1A5EE; Fri, 21 Jan 2022 14:33:28 +0100 (CET) From: Kristof Provost To: Wojciech Macek Cc: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: Re: git: 9ce46cbc95d7 - main - ip_mroute: move ip_mrouter_done outside lock Date: Fri, 21 Jan 2022 14:33:27 +0100 X-Mailer: MailMate (1.14r5852) Message-ID: In-Reply-To: <7A991003-0F24-4C8C-81FD-84F39FC78D8A@FreeBSD.org> References: <202201210519.20L5J1mJ029323@gitrepo.freebsd.org> <7A991003-0F24-4C8C-81FD-84F39FC78D8A@FreeBSD.org> List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="=_MailMate_6E9EA4D8-21EB-4CED-95BC-7FF32A12E5BC_=" ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1642772010; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=V9UhT/7/Xm4SUoWqoLqXRXmho3pXxqlQOWGg/Escky8=; b=lw56GjQp6ctJYV43lQ8dj7TUH5VL7D0QonAlfnvDpZNZxLSNI0XZOMUl0C3EIiBGxPujbA 8O8bfoR70F02saS6HucrLjnc5eZaZCKB1dTdOyL8jK24KRfcO2rVROQFG8tAW4wwnTBuje o7TUb6kkeGRXuJA+3FPQJwRWP7gSYvDY125suIX/TPHBzz2xO/6UXhHyfFE2yWIAA2eMgW c3m5sZuTfHaYHUIa/kZMDzArmXzYBtsQvjdT6GazVJCpjLRRVfzAm6DtcCcfvGu35ApEcb XjU/NdRwJJXQvc1jRiXgo/sMJLhMNcqU67QTahR4+ersHo+abCBxo8S+Twi16Q== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1642772010; a=rsa-sha256; cv=none; b=diHqe6lHSKL27DSX/79G5ErqF/6hmvVpOAOw4KIfnhBpqelJW+rRosO0PVNVLMbZgeTHQK 9MtLi/XeA8yt0DLZdR1VJMH965W6xHNAPIBB05OGwYT1stL78T/CrIqfN/V9HM0klKibQP 1g8LM3xKlNgmKAWXP/i9Q6DGbke3mR+iwD9xQ/GhMiHxW1U6rmgCM0fiHihR9z25hfeQqV /a9eK3LlYiI5Q5nwypuPxj5Li8CvDupvaHCZQsEs7T5zPGw5mTbsZ8sw5IVe/iRM9cIfEF pDcbXhzPIqLzMzC/Xe30BOdy8x+IQMv6vejw3gNWwjjzc8r2zCQQ66zfUm01/w== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N --=_MailMate_6E9EA4D8-21EB-4CED-95BC-7FF32A12E5BC_= Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable On 21 Jan 2022, at 14:01, Kristof Provost wrote: > Hi Wojciech, > > On 21 Jan 2022, at 6:19, Wojciech Macek wrote: >> The branch main has been updated by wma: >> >> URL: = >> https://cgit.FreeBSD.org/src/commit/?id=3D9ce46cbc95d7a6fccb55af0d42cb= b85c29f10639 >> >> commit 9ce46cbc95d7a6fccb55af0d42cbb85c29f10639 >> Author: Wojciech Macek >> AuthorDate: 2022-01-21 05:15:08 +0000 >> Commit: Wojciech Macek >> CommitDate: 2022-01-21 05:17:19 +0000 >> >> ip_mroute: move ip_mrouter_done outside lock >> >> X_ip_mrouter_done might sleep, which triggers INVARIANTS to >> print additional errors on the screen. >> Move it outside the lock, but provide some basic synchronization >> to avoid race condition during module uninit/unload. >> >> Obtained from: Semihalf >> Sponsored by: Stormshield > > I suspect this change causes panics like this one: = > https://ci.freebsd.org/job/FreeBSD-main-amd64-test/20437/consoleText > >> sys/netinet/ip_mroute.c | 11 ++++++++--- >> sys/netinet/ip_mroute.h | 4 +++- >> sys/netinet/raw_ip.c | 11 ++++++++--- >> 3 files changed, 19 insertions(+), 7 deletions(-) >> >> diff --git a/sys/netinet/ip_mroute.c b/sys/netinet/ip_mroute.c >> index 8cd0b2ac7449..0566048621ad 100644 >> --- a/sys/netinet/ip_mroute.c >> +++ b/sys/netinet/ip_mroute.c >> @@ -300,7 +300,7 @@ VNET_DEFINE_STATIC(struct ifnet *, = >> multicast_register_if); >> static u_long X_ip_mcast_src(int); >> static int X_ip_mforward(struct ip *, struct ifnet *, struct mbuf *, >> struct ip_moptions *); >> -static int X_ip_mrouter_done(void); >> +static int X_ip_mrouter_done(void *); >> static int X_ip_mrouter_get(struct socket *, struct sockopt *); >> static int X_ip_mrouter_set(struct socket *, struct sockopt *); >> static int X_legal_vif_num(int); >> @@ -431,7 +431,7 @@ X_ip_mrouter_set(struct socket *so, struct = >> sockopt *sopt) >> break; >> >> case MRT_DONE: >> - error =3D ip_mrouter_done(); >> + error =3D ip_mrouter_done(NULL); >> break; >> >> case MRT_ADD_VIF: >> @@ -734,7 +734,7 @@ ip_mrouter_init(struct socket *so, int version) >> * Disable multicast forwarding. >> */ >> static int >> -X_ip_mrouter_done(void) >> +X_ip_mrouter_done(void *locked) >> { >> struct ifnet *ifp; >> u_long i; >> @@ -751,6 +751,11 @@ X_ip_mrouter_done(void) >> atomic_subtract_int(&ip_mrouter_cnt, 1); >> V_mrt_api_config =3D 0; >> >> + if (locked) { >> + struct epoch_tracker *mrouter_et =3D locked; >> + MROUTER_RUNLOCK_PARAM(mrouter_et); >> + } >> + >> MROUTER_WAIT(); >> >> /* Stop and drain task queue */ >> diff --git a/sys/netinet/ip_mroute.h b/sys/netinet/ip_mroute.h >> index 65c5bdd3a025..016d026d184c 100644 >> --- a/sys/netinet/ip_mroute.h >> +++ b/sys/netinet/ip_mroute.h >> @@ -363,12 +363,14 @@ struct sockopt; >> >> extern int (*ip_mrouter_set)(struct socket *, struct sockopt *); >> extern int (*ip_mrouter_get)(struct socket *, struct sockopt *); >> -extern int (*ip_mrouter_done)(void); >> +extern int (*ip_mrouter_done)(void *); >> extern int (*mrt_ioctl)(u_long, caddr_t, int); >> >> #define MROUTER_RLOCK_TRACKER struct epoch_tracker mrouter_et >> +#define MROUTER_RLOCK_PARAM_PTR &mrouter_et >> #define MROUTER_RLOCK() epoch_enter_preempt(net_epoch_preempt, = >> &mrouter_et) >> #define MROUTER_RUNLOCK() epoch_exit_preempt(net_epoch_preempt, = >> &mrouter_et) >> +#define MROUTER_RUNLOCK_PARAM(param) epoch_exit_preempt(net_epoch_pre= empt, = >> param) >> #define MROUTER_WAIT() epoch_wait_preempt(net_epoch_preempt) >> >> #endif /* _KERNEL */ >> diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c >> index 7c495745806e..08ce848a63f7 100644 >> --- a/sys/netinet/raw_ip.c >> +++ b/sys/netinet/raw_ip.c >> @@ -119,7 +119,7 @@ VNET_DEFINE(struct socket *, ip_mrouter); >> */ >> int (*ip_mrouter_set)(struct socket *, struct sockopt *); >> int (*ip_mrouter_get)(struct socket *, struct sockopt *); >> -int (*ip_mrouter_done)(void); >> +int (*ip_mrouter_done)(void *locked); >> int (*ip_mforward)(struct ip *, struct ifnet *, struct mbuf *, >> struct ip_moptions *); >> int (*mrt_ioctl)(u_long, caddr_t, int); >> @@ -879,18 +879,23 @@ static void >> rip_detach(struct socket *so) >> { >> struct inpcb *inp; >> + MROUTER_RLOCK_TRACKER; >> >> inp =3D sotoinpcb(so); >> KASSERT(inp !=3D NULL, ("rip_detach: inp =3D=3D NULL")); >> KASSERT(inp->inp_faddr.s_addr =3D=3D INADDR_ANY, >> ("rip_detach: not closed")); >> >> + /* Disable mrouter first, lock released inside ip_mrouter_done */ >> + MROUTER_RLOCK(); >> + if (so =3D=3D V_ip_mrouter && ip_mrouter_done) >> + ip_mrouter_done(MROUTER_RLOCK_PARAM_PTR); >> + > > I believe this is the problem. > > If we do not enter ip_mrouter_done() here we=E2=80=99ll exit the functi= on = > without exiting epoch. The epoch tracker on the stack will be = > overwritten, and that could produce the panic we see in = > ci.freebsd.org. > I=E2=80=99m currently running with this patch: diff --git a/sys/netinet/ip_mroute.c b/sys/netinet/ip_mroute.c index 0566048621ad..ff68b140af7e 100644 --- a/sys/netinet/ip_mroute.c +++ b/sys/netinet/ip_mroute.c @@ -741,8 +741,13 @@ X_ip_mrouter_done(void *locked) vifi_t vifi; struct bw_upcall *bu; - if (V_ip_mrouter =3D=3D NULL) - return EINVAL; + if (V_ip_mrouter =3D=3D NULL) { + if (locked) { + struct epoch_tracker *mrouter_et =3D locked; + MROUTER_RUNLOCK_PARAM(mrouter_et); + } + return (EINVAL); + } /* * Detach/disable hooks to the reset of the system. diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c index 08ce848a63f7..4354bee3cfcc 100644 --- a/sys/netinet/raw_ip.c +++ b/sys/netinet/raw_ip.c @@ -887,9 +887,10 @@ rip_detach(struct socket *so) ("rip_detach: not closed")); /* Disable mrouter first, lock released inside ip_mrouter_done = */ - MROUTER_RLOCK(); - if (so =3D=3D V_ip_mrouter && ip_mrouter_done) + if (so =3D=3D V_ip_mrouter && ip_mrouter_done) { + MROUTER_RLOCK(); ip_mrouter_done(MROUTER_RLOCK_PARAM_PTR); + } INP_WLOCK(inp); INP_HASH_WLOCK(&V_ripcbinfo); However, it=E2=80=99s not at all clear to me what we=E2=80=99re actually = accomplishing by entering the net epoch here. As far as I can tell = that=E2=80=99s basically a no-op. Kristof --=_MailMate_6E9EA4D8-21EB-4CED-95BC-7FF32A12E5BC_= Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable

On 21 Jan 2022, at 14:01, Kristof Provost wrote:

Hi Wojciech,

On 21 Jan 2022, at 6:19, Wojciech Macek wrote:

The branch main has been updated by wma:

URL: https://cgit.FreeBSD.org/src/co= mmit/?id=3D9ce46cbc95d7a6fccb55af0d42cbb85c29f10639

commit 9ce46cbc95d7a6fccb55af0d42cbb85c29f10639
Author: Wojciech Macek <wma@FreeBSD.org>
AuthorDate: 2022-01-21 05:15:08 +0000
Commit: Wojciech Macek <wma@FreeBSD.org>
CommitDate: 2022-01-21 05:17:19 +0000

ip_mroute: move ip_mrouter_done outside lock

X_ip_mrouter_done might sleep, which triggers INVARIA= NTS to
print additional errors on the screen.
Move it outside the lock, but provide some basic synchronization
to avoid race condition during module uninit/unload.

Obtained from: Semihalf
Sponsored by: Stormshield

I suspect this change causes panics like thi= s one: https://ci.freebsd.org/job/FreeBSD-main-amd64-test/20437/c= onsoleText

sys/netinet/ip_mroute.c | 11 ++++++++---
sys/netinet/ip_mroute.h | 4 +++-
sys/netinet/raw_ip.c | 11 ++++++++---
3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/sys/netinet/ip_mroute.c b/sys/netinet/ip_mro= ute.c
index 8cd0b2ac7449..0566048621ad 100644
--- a/sys/netinet/ip_mroute.c
+++ b/sys/netinet/ip_mroute.c
@@ -300,7 +300,7 @@ VNET_DEFINE_STATIC(struct ifnet *, multicast_register= _if);
static u_long X_ip_mcast_src(int);
static int X_ip_mforward(struct ip *, struct ifnet *, struct mbuf *,
struct ip_moptions *);
-static int X_ip_mrouter_done(void);
+static int X_ip_mrouter_done(void *);
static int X_ip_mrouter_get(struct socket *, struct sockopt *);
static int X_ip_mrouter_set(struct socket *, struct sockopt *);
static int X_legal_vif_num(int);
@@ -431,7 +431,7 @@ X_ip_mrouter_set(struct socket *so, struct sockopt *s= opt)
break;

case MRT_DONE:
- error =3D ip_mrouter_done();
+ error =3D ip_mrouter_done(NULL);
break;

case MRT_ADD_VIF:
@@ -734,7 +734,7 @@ ip_mrouter_init(struct socket *so, int version)
* Disable multicast forwarding.
*/
static int
-X_ip_mrouter_done(void)
+X_ip_mrouter_done(void *locked)
{
struct ifnet *ifp;
u_long i;
@@ -751,6 +751,11 @@ X_ip_mrouter_done(void)
atomic_subtract_int(&ip_mrouter_cnt, 1);
V_mrt_api_config =3D 0;

+ if (locked) {
+ struct epoch_tracker *mrouter_et =3D locked;
+ MROUTER_RUNLOCK_PARAM(mrouter_et);
+ }
+
MROUTER_WAIT();

/* Stop and drain task queue */
diff --git a/sys/netinet/ip_mroute.h b/sys/netinet/ip_mroute.h
index 65c5bdd3a025..016d026d184c 100644
--- a/sys/netinet/ip_mroute.h
+++ b/sys/netinet/ip_mroute.h
@@ -363,12 +363,14 @@ struct sockopt;

extern int (*ip_mrouter_set)(struct socket *, struct soc= kopt *);
extern int (*ip_mrouter_get)(struct socket *, struct sockopt *);
-extern int (*ip_mrouter_done)(void);
+extern int (*ip_mrouter_done)(void *);
extern int (*mrt_ioctl)(u_long, caddr_t, int);

#define MROUTER_RLOCK_TRACKER struct epoch_tracker mrout= er_et
+#define MROUTER_RLOCK_PARAM_PTR &mrouter_et
#define MROUTER_RLOCK() epoch_enter_preempt(net_epoch_preempt, &mrou= ter_et)
#define MROUTER_RUNLOCK() epoch_exit_preempt(net_epoch_preempt, &mro= uter_et)
+#define MROUTER_RUNLOCK_PARAM(param) epoch_exit_preempt(net_epoch_preemp= t, param)
#define MROUTER_WAIT() epoch_wait_preempt(net_epoch_preempt)

#endif /* _KERNEL */
diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c
index 7c495745806e..08ce848a63f7 100644
--- a/sys/netinet/raw_ip.c
+++ b/sys/netinet/raw_ip.c
@@ -119,7 +119,7 @@ VNET_DEFINE(struct socket *, ip_mrouter);
*/
int (*ip_mrouter_set)(struct socket *, struct sockopt *);
int (*ip_mrouter_get)(struct socket *, struct sockopt *);
-int (*ip_mrouter_done)(void);
+int (*ip_mrouter_done)(void *locked);
int (*ip_mforward)(struct ip *, struct ifnet *, struct mbuf *,
struct ip_moptions *);
int (*mrt_ioctl)(u_long, caddr_t, int);
@@ -879,18 +879,23 @@ static void
rip_detach(struct socket *so)
{
struct inpcb *inp;
+ MROUTER_RLOCK_TRACKER;

inp =3D sotoinpcb(so);
KASSERT(inp !=3D NULL, ("rip_detach: inp =3D=3D NULL"));
KASSERT(inp->inp_faddr.s_addr =3D=3D INADDR_ANY,
("rip_detach: not closed"));

+ /* Disable mrouter first, lock released inside ip_mrout= er_done */
+ MROUTER_RLOCK();
+ if (so =3D=3D V_ip_mrouter && ip_mrouter_done)
+ ip_mrouter_done(MROUTER_RLOCK_PARAM_PTR);
+

I believe this is the problem.

If we do not enter ip_mrouter_done() here we=E2=80=99ll e= xit the function without exiting epoch. The epoch tracker on the stack wi= ll be overwritten, and that could produce the panic we see in ci.freebsd.= org.


I=E2=80=99m currently running with this patch:

diff --git a/sys/netinet/ip_mroute.c b/sys/netinet/ip_mrou=
te.c
index 0566048621ad..ff68b140af7e 100644
--- a/sys/netinet/ip_mroute.c
+++ b/sys/netinet/ip_mroute.c
@@ -741,8 +741,13 @@ X_ip_mrouter_done(void *locked)
     vifi_t vifi;
     struct bw_upcall *bu;

-    if (V_ip_mrouter =3D=3D NULL)
-       return EINVAL;
+    if (V_ip_mrouter =3D=3D NULL) {
+        if (locked) {
+            struct epoch_tracker *mrouter_et =3D locked;
+            MROUTER_RUNLOCK_PARAM(mrouter_et);
+        }
+        return (EINVAL);
+    }

     /*
      * Detach/disable hooks to the reset of the system.
diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c
index 08ce848a63f7..4354bee3cfcc 100644
--- a/sys/netinet/raw_ip.c
+++ b/sys/netinet/raw_ip.c
@@ -887,9 +887,10 @@ rip_detach(struct socket *so)
            ("rip_detach: not closed"));

        /* Disable mrouter first, lock released inside ip_mrouter_done */=

-       MROUTER_RLOCK();
-       if (so =3D=3D V_ip_mrouter && ip_mrouter_done)
+       if (so =3D=3D V_ip_mrouter && ip_mrouter_done) {
+               MROUTER_RLOCK();
                ip_mrouter_done(MROUTER_RLOCK_PARAM_PTR);
+       }

        INP_WLOCK(inp);
        INP_HASH_WLOCK(&V_ripcbinfo);

However, it=E2=80=99s not at all clear to me what we=E2=80= =99re actually accomplishing by entering the net epoch here. As far as I = can tell that=E2=80=99s basically a no-op.

Kristof

--=_MailMate_6E9EA4D8-21EB-4CED-95BC-7FF32A12E5BC_=--