From owner-svn-src-all@FreeBSD.ORG Fri May 2 21:23:30 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 7A9FA252; Fri, 2 May 2014 21:23:30 +0000 (UTC) Received: from mail-wi0-x231.google.com (mail-wi0-x231.google.com [IPv6:2a00:1450:400c:c05::231]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 741501CA2; Fri, 2 May 2014 21:23:29 +0000 (UTC) Received: by mail-wi0-f177.google.com with SMTP id cc10so2998286wib.4 for ; Fri, 02 May 2014 14:23:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=vZaMe0ZOIJLVjHtkxsEBhsPX89+zX+qRpCBEVHpdi1U=; b=ipuSGBdgWscd29FDJxsLG35q9kpu4gsCU7lq67mrFZ3y92cXj1RnYusqxKoNpmvcD2 +80cREKvADx+yxAJ2/ujrgFXhNBDInO4E6SARZGUslO/E6DuM+/PE+FUqO5kYZAzblZr QHqwhDLuvm8fHKprHZvIygvEb2teVmN8bwX1GVRcirqcqTkM1JWhJJgvmOEN3He1UovJ Pimxw7N5SHdO39s3PEGlhHYAwkay4pKZhrSFJtVOurXgzyKU1ypLcDy4PyHsqNxBXQlH D0Cl7MLfyWOGiMibZVpQVq5tFkcPinw5gccsIaVW28r19Ok+E8R1IPLAltmSIOynu4+U bJHg== MIME-Version: 1.0 X-Received: by 10.180.14.72 with SMTP id n8mr4691659wic.53.1399065807693; Fri, 02 May 2014 14:23:27 -0700 (PDT) Sender: asomers@gmail.com Received: by 10.194.168.130 with HTTP; Fri, 2 May 2014 14:23:27 -0700 (PDT) In-Reply-To: <5364093A.7040607@FreeBSD.org> References: <201405021624.s42GO9Hi034947@svn.freebsd.org> <5363CF6C.2000305@FreeBSD.org> <5364093A.7040607@FreeBSD.org> Date: Fri, 2 May 2014 15:23:27 -0600 X-Google-Sender-Auth: EdPcD0Nkg7tJO4tNusq2HXdiCYw Message-ID: Subject: Re: svn commit: r265232 - head/sys/net From: Alan Somers To: "Alexander V. Chernikov" Content-Type: text/plain; charset=UTF-8 Cc: "svn-src-head@freebsd.org" , "svn-src-all@freebsd.org" , "src-committers@freebsd.org" , Alan Somers X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.17 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 May 2014 21:23:30 -0000 On Fri, May 2, 2014 at 3:08 PM, Alexander V. Chernikov wrote: > On 03.05.2014 00:22, Alan Somers wrote: >> On Fri, May 2, 2014 at 11:01 AM, Alexander V. Chernikov >> wrote: >>> 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. >> >> Thanks for the retroactive review; it's good too. >> >>> >>> 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. >> >> The WUNLOCK was added in r168561 without comment. Removing it seems >> like it would work. It doesn't cause any new LORs or WITNESS >> warnings. And I can no longer reproduce the panic in lacp_req. >> However, it doesn't fix another panic in lagg_port_ioctl line 825 (my >> patch didn't either). Do you have any good ideas what to do about >> that? > Interesting. > Line numbers look a bit shifted. Is line 825 'LAGG_RUNLOCK(sc, &tracker)' ? Yes. > > Are the steps to reproduce it the same as in kern/189003? Yes. BTW, In kern/189003, I suggested reverting 253687. In reality, I didn't revert it; I commented out the sysctl lines it added in lacp_attach(). > >> >> #9 0xffffffff808cee81 in __mtx_lock_sleep (c=0xfffff800052bb648, >> tid=18446735277704396800, opts=, >> file=, line=) >> at /usr/home/alans/freebsd/head/sys/kern/kern_mutex.c:430 >> #10 0xffffffff808ced02 in __mtx_lock_flags (c=, opts=0, >> file=0xffffffff80f6dd8c >> "/usr/home/alans/freebsd/head/sys/kern/kern_rmlock.c", line=407) at >> /usr/home/alans/freebsd/head/sys/kern/kern_mutex.c:223 >> #11 0xffffffff808e0032 in _rm_rlock (rm=0xfffff800052bb608, >> tracker=0xfffffe0097751918, trylock=) >> at /usr/home/alans/freebsd/head/sys/kern/kern_rmlock.c:407 >> #12 0xffffffff808e0812 in _rm_rlock_debug (rm=0xfffff800052bb608, >> tracker=0xfffffe0097751918, trylock=0, >> file=0xffffffff81c1dd13 >> "/usr/home/alans/freebsd/head/sys/modules/if_lagg/../../net/if_lagg.c", >> line=825) >> at /usr/home/alans/freebsd/head/sys/kern/kern_rmlock.c:659 >> #13 0xffffffff81c19f62 in lagg_port_ioctl (ifp=0xfffff8000590d800, >> cmd=, data=0xfffff80005582c00 "tap0") >> at /usr/home/alans/freebsd/head/sys/modules/if_lagg/../../net/if_lagg.c:825 >> #14 0xffffffff809ae407 in ifioctl (so=, cmd=3225971084, >> data=0xfffff80005582c00 "tap0", td=) >> at /usr/home/alans/freebsd/head/sys/net/if.c:2643 >> #15 0xffffffff8093b9fb in kern_ioctl (td=, >> fd=, com=) at file.h:323 >> #16 0xffffffff8093b77c in sys_ioctl (td=0xfffff800053cc000, >> uap=0xfffffe0097751b80) >> at /usr/home/alans/freebsd/head/sys/kern/sys_generic.c:702 >> >