From owner-svn-src-head@FreeBSD.ORG Fri May 2 17:02:16 2014 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 3F3E9974; Fri, 2 May 2014 17:02:16 +0000 (UTC) Received: from mail.ipfw.ru (mail.ipfw.ru [IPv6:2a01:4f8:120:6141::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 001821351; Fri, 2 May 2014 17:02:15 +0000 (UTC) Received: from [2a02:6b8:0:401:222:4dff:fe50:cd2f] (helo=ptichko.yndx.net) by mail.ipfw.ru with esmtpsa (TLSv1:CAMELLIA256-SHA:256) (Exim 4.76 (FreeBSD)) (envelope-from ) id 1WgCx4-0003Ae-Vk; Fri, 02 May 2014 16:52:31 +0400 Message-ID: <5363CF6C.2000305@FreeBSD.org> Date: Fri, 02 May 2014 21:01:32 +0400 From: "Alexander V. Chernikov" User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.0.1 MIME-Version: 1.0 To: Alan Somers , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r265232 - head/sys/net References: <201405021624.s42GO9Hi034947@svn.freebsd.org> In-Reply-To: <201405021624.s42GO9Hi034947@svn.freebsd.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 May 2014 17:02:16 -0000 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, > >