Date: Thu, 6 Oct 2005 10:56:02 +1300 From: Andrew Thompson <thompsa@freebsd.org> To: Brooks Davis <brooks@one-eyed-alien.net> Cc: Brooks Davis <brooks@FreeBSD.org>, Pawel Jakub Dawidek <pjd@FreeBSD.org>, FreeBSD Current <current@FreeBSD.org> Subject: Re: panic: ifc_free_unit: bit is already cleared Message-ID: <20051005215602.GA76375@heff.fud.org.nz> In-Reply-To: <20051005205515.GA30350@odin.ac.hmc.edu> References: <20051005024903.GA72743@heff.fud.org.nz> <20051005203639.GA20552@garage.freebsd.pl> <20051005205515.GA30350@odin.ac.hmc.edu>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20051005215602.GA76375>