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>
index | next in thread | previous in thread | raw e-mail
[-- Attachment #1 --] 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żytkownik Kristof Provost <kp@freebsd.org> napisał: > 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=9ce46cbc95d7a6fccb55af0d42cbb85c29f10639 > > 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 = ip_mrouter_done(); > + error = 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 = 0; > > + if (locked) { > + struct epoch_tracker *mrouter_et = 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 = sotoinpcb(so); > KASSERT(inp != NULL, ("rip_detach: inp == NULL")); > KASSERT(inp->inp_faddr.s_addr == INADDR_ANY, > ("rip_detach: not closed")); > > + /* Disable mrouter first, lock released inside ip_mrouter_done */ > + MROUTER_RLOCK(); > + if (so == 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’ll exit the function 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’m 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 == NULL) > - return EINVAL; > + if (V_ip_mrouter == NULL) { > + if (locked) { > + struct epoch_tracker *mrouter_et = 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 == V_ip_mrouter && ip_mrouter_done) > + if (so == 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’s not at all clear to me what we’re actually accomplishing by > entering the net epoch here. As far as I can tell that’s basically a no-op. > > Kristof > [-- Attachment #2 --] <div dir="auto"><div>I will prepare a fix later today. Basically, the whole "if" condition must be inside rwlock block to avoid race during module unload. <div dir="auto">Maybe adding else and rwunlock in rip_detach will suffice.... I need to try it out.</div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">pt., 21 sty 2022, 14:33 użytkownik Kristof Provost <<a href="mailto:kp@freebsd.org">kp@freebsd.org</a>> napisał:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><u></u> <div><div style="font-family:sans-serif"><div style="white-space:normal"> <p dir="auto">On 21 Jan 2022, at 14:01, Kristof Provost wrote:</p> </div><div style="white-space:normal"><blockquote style="margin:0 0 5px;padding-left:5px;border-left:2px solid #136bce;color:#136bce"><p dir="auto">Hi Wojciech,</p> <p dir="auto">On 21 Jan 2022, at 6:19, Wojciech Macek wrote:</p> <blockquote style="margin:0 0 5px;padding-left:5px;border-left:2px solid #136bce;border-left-color:#4b89cf;color:#4b89cf"><p dir="auto">The branch main has been updated by wma:</p> <p dir="auto">URL: <a href="https://cgit.FreeBSD.org/src/commit/?id=9ce46cbc95d7a6fccb55af0d42cbb85c29f10639" target="_blank" rel="noreferrer">https://cgit.FreeBSD.org/src/commit/?id=9ce46cbc95d7a6fccb55af0d42cbb85c29f10639</a></p>; <p dir="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="auto"> ip_mroute: move ip_mrouter_done outside lock</p> <p dir="auto"> X_ip_mrouter_done might sleep, which triggers INVARIANTS 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="auto"> Obtained from: Semihalf <br> Sponsored by: Stormshield</p> </blockquote><p dir="auto">I suspect this change causes panics like this one: <a href="https://ci.freebsd.org/job/FreeBSD-main-amd64-test/20437/consoleText" target="_blank" rel="noreferrer">https://ci.freebsd.org/job/FreeBSD-main-amd64-test/20437/consoleText</a></p>; <blockquote style="margin:0 0 5px;padding-left:5px;border-left:2px solid #136bce;border-left-color:#4b89cf;color:#4b89cf"><p dir="auto"> 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="auto">diff --git a/sys/netinet/ip_mroute.c b/sys/netinet/ip_mroute.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 *sopt) <br> break;</p> <p dir="auto"> case MRT_DONE: <br> - error = ip_mrouter_done(); <br> + error = ip_mrouter_done(NULL); <br> break;</p> <p dir="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 = 0;</p> <p dir="auto">+ if (locked) { <br> + struct epoch_tracker *mrouter_et = locked; <br> + MROUTER_RUNLOCK_PARAM(mrouter_et); <br> + } <br> + <br> MROUTER_WAIT();</p> <p dir="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="auto"> extern int (*ip_mrouter_set)(struct socket *, struct sockopt *); <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="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, &mrouter_et) <br> #define MROUTER_RUNLOCK() epoch_exit_preempt(net_epoch_preempt, &mrouter_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="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="auto"> inp = sotoinpcb(so); <br> KASSERT(inp != NULL, ("rip_detach: inp == NULL")); <br> KASSERT(inp->inp_faddr.s_addr == INADDR_ANY, <br> ("rip_detach: not closed"));</p> <p dir="auto">+ /* Disable mrouter first, lock released inside ip_mrouter_done */ <br> + MROUTER_RLOCK(); <br> + if (so == V_ip_mrouter && ip_mrouter_done) <br> + ip_mrouter_done(MROUTER_RLOCK_PARAM_PTR); <br> +</p> </blockquote><p dir="auto">I believe this is the problem.</p> <p dir="auto">If we do not enter ip_mrouter_done() here we’ll exit the function without exiting epoch. The epoch tracker on the stack will be overwritten, and that could produce the panic we see in <a href="http://ci.freebsd.org" target="_blank" rel="noreferrer">ci.freebsd.org</a>.</p> <br></blockquote></div> <div style="white-space:normal"> <p dir="auto">I’m currently running with this patch:</p> <pre style="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_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 == NULL) - return EINVAL; + if (V_ip_mrouter == NULL) { + if (locked) { + struct epoch_tracker *mrouter_et = 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 == V_ip_mrouter && ip_mrouter_done) + if (so == 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="auto">However, it’s not at all clear to me what we’re actually accomplishing by entering the net epoch here. As far as I can tell that’s basically a no-op.</p> <p dir="auto">Kristof</p> </div></div></div> </blockquote></div></div></div>help
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANsEV8cmZcQKh3U8w2S37cg9heFd=icuEmF7dVwrE486OM3Yug>