From nobody Sat Jan 22 07:50:54 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 8AFDE1959A6E for ; Sat, 22 Jan 2022 07:51:08 +0000 (UTC) (envelope-from wma@semihalf.com) Received: from mail-yb1-xb36.google.com (mail-yb1-xb36.google.com [IPv6:2607:f8b0:4864:20::b36]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4JgpMv4KFfz4sdB for ; Sat, 22 Jan 2022 07:51:07 +0000 (UTC) (envelope-from wma@semihalf.com) Received: by mail-yb1-xb36.google.com with SMTP id m6so34271798ybc.9 for ; Fri, 21 Jan 2022 23:51:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=5dTIz1k7/xJze26p1uOII+U531yPKUffS3JRwCdtLno=; b=1Vbw9ukScwdNKBaBdEvrxLfEJXqiLoFhROkPvHYGd+b8NiR9gXCiJLMD66gGqvXxEq YELFLV4dUZZdrbKZ9vVozIgTcrpQnUR9H3Eu3SoTIX47eWTWpSxM3Tnwg5lpxoNqo/G9 WysHoSMrPw/yyBzxbv+P9LLJdsXd54jGOvy1iclbzSiYx2WtNRZzAEpHO7/Gi5kWnSRy 64qJoIKK3fkCgj1tqPokp96E3W8lVYbPAWAe5oalB7gm1aMsSZroA8bgKex393aiWk2q Zi90oN5yaljDIiry330aUxFNtZfWvcR5Z7IjV0yeQ8+kN/2hD3ljKjQLvuAhiVxLkgOu fWAw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=5dTIz1k7/xJze26p1uOII+U531yPKUffS3JRwCdtLno=; b=yEZLJa7kFLk+djuSusG/NkT5jVyJzv3z1bb7+rl6YxBfGEY7qwec8li41aEue67aME Fb0rULhxtdTM3Dtow/3dQudTBz0Ak2u5+hfjDyd/3IBT6LCslOmI4y8UFoTNxSs3uEEq rWgQKSBpaYQ3b9n7W5aNyOQX7uMZeJ788tF/NpHQ4/oYzExozFSo9bFRR0pJXMyStIU8 b9yJvXwh5YxD3rl0cV6eRLXsqr4Ff7mOlhZmJ41ARMRUJvKGzbAg2BzW+eNPQbapYw48 zxWh6nJT7yNH2ulGcYYhZk/ydhc1EsKeF3oiT9MhXiyqHa/CeIyFI+5mnlNbKP/AS58R mbAQ== X-Gm-Message-State: AOAM530mS78+IzPQFDz+3NmfFEtKFAE/GULGBWQCboSlZnmvbBfbtYtB D4i/6kuCUAwNKHcNNRsZac1Jyke2g7IX4sA/y8cPwQ== X-Google-Smtp-Source: ABdhPJyYP5pMTrsnK3PDfQIkX63q+y8ishC33qeAxnCd58ZKYGfONXw73JAx+BIOmlrnxujC4WyC0MKjnigfVUcGhA0= X-Received: by 2002:a5b:f0e:: with SMTP id x14mr11081007ybr.671.1642837866909; Fri, 21 Jan 2022 23:51:06 -0800 (PST) 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 References: <202201210519.20L5J1mJ029323@gitrepo.freebsd.org> <7A991003-0F24-4C8C-81FD-84F39FC78D8A@FreeBSD.org> In-Reply-To: From: Wojciech Macek Date: Sat, 22 Jan 2022 08:50:54 +0100 Message-ID: Subject: Re: git: 9ce46cbc95d7 - main - ip_mroute: move ip_mrouter_done outside lock To: Kristof Provost Cc: Wojciech Macek , src-committers , "" , dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="0000000000007b28f305d626fff3" X-Rspamd-Queue-Id: 4JgpMv4KFfz4sdB X-Spamd-Bar: / Authentication-Results: mx1.freebsd.org; dkim=pass header.d=semihalf-com.20210112.gappssmtp.com header.s=20210112 header.b=1Vbw9ukS; dmarc=none; spf=none (mx1.freebsd.org: domain of wma@semihalf.com has no SPF policy when checking 2607:f8b0:4864:20::b36) smtp.mailfrom=wma@semihalf.com X-Spamd-Result: default: False [0.70 / 15.00]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[semihalf-com.20210112.gappssmtp.com:s=20210112]; FREEFALL_USER(0.00)[wma]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[dev-commits-src-all@freebsd.org]; DMARC_NA(0.00)[semihalf.com]; NEURAL_SPAM_MEDIUM(1.00)[0.999]; RCPT_COUNT_FIVE(0.00)[5]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[semihalf-com.20210112.gappssmtp.com:+]; NEURAL_SPAM_LONG(1.00)[1.000]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::b36:from]; NEURAL_HAM_SHORT(-1.00)[-1.000]; MLMMJ_DEST(0.00)[dev-commits-src-all]; R_SPF_NA(0.00)[no SPF record]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; RCVD_COUNT_TWO(0.00)[2]; RCVD_TLS_ALL(0.00)[] X-ThisMailContainsUnwantedMimeParts: N --0000000000007b28f305d626fff3 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable I will prepare a fix later today. Basically, the whole "if" condition must be inside rwlock block to avoid race during module unload. Maybe adding else and rwunlock in rip_detach will suffice.... I need to try it out. pt., 21 sty 2022, 14:33 u=C5=BCytkownik Kristof Provost na= pisa=C5=82: > 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=3D9ce46cbc95d7a6fccb55af0d42cbb85= c29f10639 > > 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_preempt, 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 function= without > exiting epoch. The epoch tracker on the stack will be overwritten, and th= at > 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 basicall= y a no-op. > > Kristof > --0000000000007b28f305d626fff3 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I will prepare a fix later today. Basically, the who= le "if" condition must be inside rwlock block to avoid race durin= g module unload.=C2=A0
Maybe adding else and rwunlock in r= ip_detach will suffice.... I need to try it out.


pt., 21 sty 2022, 14:33= u=C5=BCytkownik Kristof Provost <kp@f= reebsd.org> napisa=C5=82:

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=3D9ce46cbc95d7a6fccb55af0d42cbb8= 5c29f10639

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 INVARIANT= S 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/netin= et/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_mrout= e.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_i= f);
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 *sop= t)
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 socko= pt *);
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, &mroute= r_et)
#define MROUTER_RUNLOCK() epoch_exit_preempt(net_epoch_preempt, &mrout= er_et)
+#define MROUTER_RUNLOCK_PARAM(param) epoch_exit_preempt(net_epoch_preempt,= 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 exi= t the function without exiting epoch. The epoch tracker on the stack will b= e overwritten, and that could produce the panic we see in ci.freebsd.org.

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

dif=
f --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 ca= n tell that=E2=80=99s basically a no-op.

Kristof

--0000000000007b28f305d626fff3--