From owner-freebsd-current@FreeBSD.ORG Wed Oct 5 21:56:04 2005 Return-Path: X-Original-To: current@FreeBSD.org Delivered-To: freebsd-current@FreeBSD.ORG Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 48AC016A41F; Wed, 5 Oct 2005 21:56:04 +0000 (GMT) (envelope-from thompsa@freebsd.org) Received: from heff.fud.org.nz (60-234-149-201.bitstream.orcon.net.nz [60.234.149.201]) by mx1.FreeBSD.org (Postfix) with ESMTP id 91ACA43D45; Wed, 5 Oct 2005 21:56:03 +0000 (GMT) (envelope-from thompsa@freebsd.org) Received: by heff.fud.org.nz (Postfix, from userid 1001) id 352531CCD4; Thu, 6 Oct 2005 10:56:02 +1300 (NZDT) Date: Thu, 6 Oct 2005 10:56:02 +1300 From: Andrew Thompson To: Brooks Davis Message-ID: <20051005215602.GA76375@heff.fud.org.nz> Mail-Followup-To: Andrew Thompson , Brooks Davis , Pawel Jakub Dawidek , Brooks Davis , FreeBSD Current References: <20051005024903.GA72743@heff.fud.org.nz> <20051005203639.GA20552@garage.freebsd.pl> <20051005205515.GA30350@odin.ac.hmc.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20051005205515.GA30350@odin.ac.hmc.edu> User-Agent: Mutt/1.4.2.1i Cc: Brooks Davis , Pawel Jakub Dawidek , FreeBSD Current Subject: Re: panic: ifc_free_unit: bit is already cleared X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Oct 2005 21:56:04 -0000 On Wed, Oct 05, 2005 at 01:55:15PM -0700, Brooks Davis wrote: > On Wed, Oct 05, 2005 at 10:36:39PM +0200, Pawel Jakub Dawidek wrote: > > On Wed, Oct 05, 2005 at 03:49:03PM +1300, Andrew Thompson wrote: > > +> Hi, > > +> > > +> I have found a repeatable panic with network device cloning, unfortunatly I am > > +> unable to dump on this box. This is sparc64 with a 2 day old current. > > > > The order is wrong in vlan_modevent(). > > > > if_clone_detach() is freeing ifc_units field, so ifc_free_unit() should not > > be called after that. > > > > This patch should fix the problem: > > > > http://people.freebsd.org/~pjd/patches/if_vlan.c.patch > > Yes. This does introduce a race in that a new interface could > be created between the vlan_clone_destroy loop and the call to > if_clone_detach. It's going to be hard to trigger, but it probably > should be fixed. Since cloning isn't performance critical, I think > adding a dead flag to the clone structure and failing all attempts once > the flag is set. I think this patch fixes the problem while avoiding the race. It changes ifc->ifc_refcnt to count the number of attached interfaces. Andrew Index: if_clone.c =================================================================== RCS file: /home/ncvs/src/sys/net/if_clone.c,v retrieving revision 1.6 diff -u -p -r1.6 if_clone.c --- if_clone.c 24 Feb 2005 13:14:41 -0000 1.6 +++ if_clone.c 5 Oct 2005 21:49:13 -0000 @@ -124,7 +124,6 @@ if_clone_create(char *name, size_t len) IF_CLONERS_LOCK(); LIST_FOREACH(ifc, &if_cloners, ifc_list) { if (ifc->ifc_match(ifc, name)) { - IF_CLONE_ADDREF(ifc); break; } } @@ -134,7 +133,6 @@ if_clone_create(char *name, size_t len) return (EINVAL); err = (*ifc->ifc_create)(ifc, name, len); - IF_CLONE_REMREF(ifc); return (err); } @@ -156,7 +154,6 @@ if_clone_destroy(const char *name) IF_CLONERS_LOCK(); LIST_FOREACH(ifc, &if_cloners, ifc_list) { if (strcmp(ifc->ifc_name, ifp->if_dname) == 0) { - IF_CLONE_ADDREF(ifc); break; } } @@ -172,7 +169,6 @@ if_clone_destroy(const char *name) err = (*ifc->ifc_destroy)(ifc, ifp); done: - IF_CLONE_REMREF(ifc); return (err); } @@ -353,6 +349,7 @@ ifc_alloc_unit(struct if_clone *ifc, int * Allocate the unit in the bitmap. */ ifc->ifc_units[bytoff] |= (1 << bitoff); + IF_CLONE_ADDREF_LOCKED(ifc); done: IF_CLONE_UNLOCK(ifc); @@ -375,7 +372,7 @@ ifc_free_unit(struct if_clone *ifc, int KASSERT((ifc->ifc_units[bytoff] & (1 << bitoff)) != 0, ("%s: bit is already cleared", __func__)); ifc->ifc_units[bytoff] &= ~(1 << bitoff); - IF_CLONE_UNLOCK(ifc); + IF_CLONE_REMREF_LOCKED(ifc); /* releases lock */ } void