Date: Sat, 22 Jan 2022 08:50:54 +0100 From: Wojciech Macek <wma@semihalf.com> To: Kristof Provost <kp@freebsd.org> Cc: Wojciech Macek <wma@freebsd.org>, src-committers <src-committers@freebsd.org>, "<dev-commits-src-all@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 Message-ID: <CANsEV8cmZcQKh3U8w2S37cg9heFd=icuEmF7dVwrE486OM3Yug@mail.gmail.com> In-Reply-To: <C25EE57B-BE5A-4BC8-AD00-C8D0D582D073@FreeBSD.org> References: <202201210519.20L5J1mJ029323@gitrepo.freebsd.org> <7A991003-0F24-4C8C-81FD-84F39FC78D8A@FreeBSD.org> <C25EE57B-BE5A-4BC8-AD00-C8D0D582D073@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <kp@freebsd.org> 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 <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 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 <div dir=3D"auto"><div>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<div dir=3D"auto">Maybe adding else and rwunlock in r= ip_detach will suffice.... I need to try it out.</div><br><br><div class=3D= "gmail_quote"><div dir=3D"ltr" class=3D"gmail_attr">pt., 21 sty 2022, 14:33= u=C5=BCytkownik Kristof Provost <<a href=3D"mailto:kp@freebsd.org">kp@f= reebsd.org</a>> napisa=C5=82:<br></div><blockquote class=3D"gmail_quote"= style=3D"margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><u= ></u> <div><div style=3D"font-family:sans-serif"><div style=3D"white-space:normal= "> <p dir=3D"auto">On 21 Jan 2022, at 14:01, Kristof Provost wrote:</p> </div><div style=3D"white-space:normal"><blockquote style=3D"margin:0 0 5px= ;padding-left:5px;border-left:2px solid #136bce;color:#136bce"><p dir=3D"au= to">Hi Wojciech,</p> <p dir=3D"auto">On 21 Jan 2022, at 6:19, Wojciech Macek wrote:</p> <blockquote style=3D"margin:0 0 5px;padding-left:5px;border-left:2px solid = #136bce;border-left-color:#4b89cf;color:#4b89cf"><p dir=3D"auto">The branch= main has been updated by wma:</p> <p dir=3D"auto">URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3D9= ce46cbc95d7a6fccb55af0d42cbb85c29f10639" target=3D"_blank" rel=3D"noreferre= r">https://cgit.FreeBSD.org/src/commit/?id=3D9ce46cbc95d7a6fccb55af0d42cbb8= 5c29f10639</a></p> <p dir=3D"auto">commit 9ce46cbc95d7a6fccb55af0d42cbb85c29f10639 <br> Author: Wojciech Macek <wma@FreeBSD.org> <br> AuthorDate: 2022-01-21 05:15:08 +0000 <br> Commit: Wojciech Macek <wma@FreeBSD.org> <br> CommitDate: 2022-01-21 05:17:19 +0000</p> <p dir=3D"auto"> ip_mroute: move ip_mrouter_done outside lock</p> <p dir=3D"auto"> X_ip_mrouter_done might sleep, which triggers INVARIANT= S to <br> print additional errors on the screen. <br> Move it outside the lock, but provide some basic synchronization <br> to avoid race condition during module uninit/unload.</p> <p dir=3D"auto"> Obtained from: Semihalf <br> Sponsored by: Stormshield</p> </blockquote><p dir=3D"auto">I suspect this change causes panics like this = one: <a href=3D"https://ci.freebsd.org/job/FreeBSD-main-amd64-test/20437/co= nsoleText" target=3D"_blank" rel=3D"noreferrer">https://ci.freebsd.org/job/= FreeBSD-main-amd64-test/20437/consoleText</a></p> <blockquote style=3D"margin:0 0 5px;padding-left:5px;border-left:2px solid = #136bce;border-left-color:#4b89cf;color:#4b89cf"><p dir=3D"auto"> sys/netin= et/ip_mroute.c | 11 ++++++++--- <br> sys/netinet/ip_mroute.h | 4 +++- <br> sys/netinet/raw_ip.c | 11 ++++++++--- <br> 3 files changed, 19 insertions(+), 7 deletions(-)</p> <p dir=3D"auto">diff --git a/sys/netinet/ip_mroute.c b/sys/netinet/ip_mrout= e.c <br> index 8cd0b2ac7449..0566048621ad 100644 <br> --- a/sys/netinet/ip_mroute.c <br> +++ b/sys/netinet/ip_mroute.c <br> @@ -300,7 +300,7 @@ VNET_DEFINE_STATIC(struct ifnet *, multicast_register_i= f); <br> static u_long X_ip_mcast_src(int); <br> static int X_ip_mforward(struct ip *, struct ifnet *, struct mbuf *, <br> struct ip_moptions *); <br> -static int X_ip_mrouter_done(void); <br> +static int X_ip_mrouter_done(void *); <br> static int X_ip_mrouter_get(struct socket *, struct sockopt *); <br> static int X_ip_mrouter_set(struct socket *, struct sockopt *); <br> static int X_legal_vif_num(int); <br> @@ -431,7 +431,7 @@ X_ip_mrouter_set(struct socket *so, struct sockopt *sop= t) <br> break;</p> <p dir=3D"auto"> case MRT_DONE: <br> - error =3D ip_mrouter_done(); <br> + error =3D ip_mrouter_done(NULL); <br> break;</p> <p dir=3D"auto"> case MRT_ADD_VIF: <br> @@ -734,7 +734,7 @@ ip_mrouter_init(struct socket *so, int version) <br> * Disable multicast forwarding. <br> */ <br> static int <br> -X_ip_mrouter_done(void) <br> +X_ip_mrouter_done(void *locked) <br> { <br> struct ifnet *ifp; <br> u_long i; <br> @@ -751,6 +751,11 @@ X_ip_mrouter_done(void) <br> atomic_subtract_int(&ip_mrouter_cnt, 1); <br> V_mrt_api_config =3D 0;</p> <p dir=3D"auto">+ if (locked) { <br> + struct epoch_tracker *mrouter_et =3D locked; <br> + MROUTER_RUNLOCK_PARAM(mrouter_et); <br> + } <br> + <br> MROUTER_WAIT();</p> <p dir=3D"auto"> /* Stop and drain task queue */ <br> diff --git a/sys/netinet/ip_mroute.h b/sys/netinet/ip_mroute.h <br> index 65c5bdd3a025..016d026d184c 100644 <br> --- a/sys/netinet/ip_mroute.h <br> +++ b/sys/netinet/ip_mroute.h <br> @@ -363,12 +363,14 @@ struct sockopt;</p> <p dir=3D"auto"> extern int (*ip_mrouter_set)(struct socket *, struct socko= pt *); <br> extern int (*ip_mrouter_get)(struct socket *, struct sockopt *); <br> -extern int (*ip_mrouter_done)(void); <br> +extern int (*ip_mrouter_done)(void *); <br> extern int (*mrt_ioctl)(u_long, caddr_t, int);</p> <p dir=3D"auto"> #define MROUTER_RLOCK_TRACKER struct epoch_tracker mrouter= _et <br> +#define MROUTER_RLOCK_PARAM_PTR &mrouter_et <br> #define MROUTER_RLOCK() epoch_enter_preempt(net_epoch_preempt, &mroute= r_et) <br> #define MROUTER_RUNLOCK() epoch_exit_preempt(net_epoch_preempt, &mrout= er_et) <br> +#define MROUTER_RUNLOCK_PARAM(param) epoch_exit_preempt(net_epoch_preempt,= param) <br> #define MROUTER_WAIT() epoch_wait_preempt(net_epoch_preempt)</p> <p dir=3D"auto"> #endif /* _KERNEL */ <br> diff --git a/sys/netinet/raw_ip.c b/sys/netinet/raw_ip.c <br> index 7c495745806e..08ce848a63f7 100644 <br> --- a/sys/netinet/raw_ip.c <br> +++ b/sys/netinet/raw_ip.c <br> @@ -119,7 +119,7 @@ VNET_DEFINE(struct socket *, ip_mrouter); <br> */ <br> int (*ip_mrouter_set)(struct socket *, struct sockopt *); <br> int (*ip_mrouter_get)(struct socket *, struct sockopt *); <br> -int (*ip_mrouter_done)(void); <br> +int (*ip_mrouter_done)(void *locked); <br> int (*ip_mforward)(struct ip *, struct ifnet *, struct mbuf *, <br> struct ip_moptions *); <br> int (*mrt_ioctl)(u_long, caddr_t, int); <br> @@ -879,18 +879,23 @@ static void <br> rip_detach(struct socket *so) <br> { <br> struct inpcb *inp; <br> + MROUTER_RLOCK_TRACKER;</p> <p dir=3D"auto"> inp =3D sotoinpcb(so); <br> KASSERT(inp !=3D NULL, ("rip_detach: inp =3D=3D NULL")); <br> KASSERT(inp->inp_faddr.s_addr =3D=3D INADDR_ANY, <br> ("rip_detach: not closed"));</p> <p dir=3D"auto">+ /* Disable mrouter first, lock released inside ip_mrouter= _done */ <br> + MROUTER_RLOCK(); <br> + if (so =3D=3D V_ip_mrouter && ip_mrouter_done) <br> + ip_mrouter_done(MROUTER_RLOCK_PARAM_PTR); <br> +</p> </blockquote><p dir=3D"auto">I believe this is the problem.</p> <p dir=3D"auto">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 <a href=3D"http:/= /ci.freebsd.org" target=3D"_blank" rel=3D"noreferrer">ci.freebsd.org</a>.</= p> <br></blockquote></div> <div style=3D"white-space:normal"> <p dir=3D"auto">I=E2=80=99m currently running with this patch:</p> <pre style=3D"margin-left:15px;margin-right:15px;padding:5px;border:thin so= lid gray;overflow-x:auto;max-width:90vw;background-color:#e4e4e4"><code>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); </code></pre> <p dir=3D"auto">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.</p> <p dir=3D"auto">Kristof</p> </div></div></div> </blockquote></div></div></div> --0000000000007b28f305d626fff3--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANsEV8cmZcQKh3U8w2S37cg9heFd=icuEmF7dVwrE486OM3Yug>