Date: Fri, 02 May 2014 21:01:32 +0400 From: "Alexander V. Chernikov" <melifaro@FreeBSD.org> To: Alan Somers <asomers@FreeBSD.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r265232 - head/sys/net Message-ID: <5363CF6C.2000305@FreeBSD.org> In-Reply-To: <201405021624.s42GO9Hi034947@svn.freebsd.org> References: <201405021624.s42GO9Hi034947@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 02.05.2014 20:24, Alan Somers wrote: > Author: asomers > Date: Fri May 2 16:24:09 2014 > New Revision: 265232 > URL: http://svnweb.freebsd.org/changeset/base/265232 > > Log: > Fix a panic caused by doing "ifconfig -am" while a lagg is being destroyed. > The thread that is destroying the lagg has already set sc->sc_psc=NULL when > the "ifconfig -am" thread gets to lacp_req(). It tries to dereference > sc->sc_psc and panics. The solution is for lacp_req() to check the value of > sc->sc_psc. If NULL, harmlessly return an lacp_opreq structure full of > zeros. Full details in GNATS. Sorry, it looks like I've missed -net@ discussion. While this patch minimizes panics, they still can occur. If one thread performs lagg_clone_destroy() -> lagg_lacp_detach() (1) -> lacp_detach(), executing struct lacp_softc *lsc = LACP_SOFTC(sc); (e.g. one line before sc_psc = NULL assignment) At the same moment, another thread (initiated by ifconfig) executes struct lacp_softc *lsc = LACP_SOFTC(sc); Then, thread #1 goes further to LACP_LOCK_DESTROY(lsc); free(lsc, M_DEVBUF); After that, thread #2 performs bzero(req, sizeof(struct lacp_opreq)); passes lsc check for NULL and crashes on destroyed mutex. Briefly looking, we can remove WUNLOCK() before lacp_detach() in lagg_lacp_detach() (I'm unsure about the reasons why do we need unlock here). lacp_req() is already protected by at least LAGG_RLOCK() so we should get consistent view of sc->sc_psc. > > PR: kern/189003 > Reviewed by: timeout on freebsd-net@ > MFC after: 3 weeks > Sponsored by: Spectra Logic Corporation > > Modified: > head/sys/net/ieee8023ad_lacp.c > > Modified: head/sys/net/ieee8023ad_lacp.c > ============================================================================== > --- head/sys/net/ieee8023ad_lacp.c Fri May 2 16:15:34 2014 (r265231) > +++ head/sys/net/ieee8023ad_lacp.c Fri May 2 16:24:09 2014 (r265232) > @@ -590,10 +590,20 @@ lacp_req(struct lagg_softc *sc, caddr_t > { > struct lacp_opreq *req = (struct lacp_opreq *)data; > struct lacp_softc *lsc = LACP_SOFTC(sc); > - struct lacp_aggregator *la = lsc->lsc_active_aggregator; > + struct lacp_aggregator *la; > > - LACP_LOCK(lsc); > bzero(req, sizeof(struct lacp_opreq)); > + > + /* > + * If the LACP softc is NULL, return with the opreq structure full of > + * zeros. It is normal for the softc to be NULL while the lagg is > + * being destroyed. > + */ > + if (NULL == lsc) > + return; > + > + la = lsc->lsc_active_aggregator; > + LACP_LOCK(lsc); > if (la != NULL) { > req->actor_prio = ntohs(la->la_actor.lip_systemid.lsi_prio); > memcpy(&req->actor_mac, &la->la_actor.lip_systemid.lsi_mac, > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5363CF6C.2000305>