Date: Fri, 21 Jan 2022 14:33:27 +0100 From: Kristof Provost <kp@FreeBSD.org> To: Wojciech Macek <wma@FreeBSD.org> 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 Message-ID: <C25EE57B-BE5A-4BC8-AD00-C8D0D582D073@FreeBSD.org> In-Reply-To: <7A991003-0F24-4C8C-81FD-84F39FC78D8A@FreeBSD.org> References: <202201210519.20L5J1mJ029323@gitrepo.freebsd.org> <7A991003-0F24-4C8C-81FD-84F39FC78D8A@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--=_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 <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_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 <!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">On 21 Jan 2022, at 14:01, Kristof Provost 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">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"au= to">The branch main has been updated by wma:</p> <p dir=3D"auto">URL: <a href=3D"https://cgit.FreeBSD.org/src/commit/?id=3D= 9ce46cbc95d7a6fccb55af0d42cbb85c29f10639">https://cgit.FreeBSD.org/src/co= mmit/?id=3D9ce46cbc95d7a6fccb55af0d42cbb85c29f10639</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 INVARIA= NTS 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 thi= s one: <a href=3D"https://ci.freebsd.org/job/FreeBSD-main-amd64-test/2043= 7/consoleText">https://ci.freebsd.org/job/FreeBSD-main-amd64-test/20437/c= onsoleText</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"au= to"> sys/netinet/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_mro= ute.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= _if); <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 *s= opt) <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 soc= kopt *); <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 mrout= er_et <br> +#define MROUTER_RLOCK_PARAM_PTR &mrouter_et <br> #define MROUTER_RLOCK() epoch_enter_preempt(net_epoch_preempt, &mrou= ter_et) <br> #define MROUTER_RUNLOCK() epoch_exit_preempt(net_epoch_preempt, &mro= uter_et) <br> +#define MROUTER_RUNLOCK_PARAM(param) epoch_exit_preempt(net_epoch_preemp= t, 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_mrout= er_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 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.</p> <br></blockquote></div> <div class=3D"markdown" 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 solid gray; overflow-x: auto; max-width: 90vw; background-color: #= E4E4E4;"><code>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); </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 = can tell that=E2=80=99s basically a no-op.</p> <p dir=3D"auto">Kristof</p> </div></div></body> </html> --=_MailMate_6E9EA4D8-21EB-4CED-95BC-7FF32A12E5BC_=--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?C25EE57B-BE5A-4BC8-AD00-C8D0D582D073>