Re: panic: ifc_free_unit: bit is already cleared

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
_at__at_ -124,7 +124,6 _at__at_ 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;
 		}
 	}
_at__at_ -134,7 +133,6 _at__at_ if_clone_create(char *name, size_t len)
 		return (EINVAL);
 
 	err = (*ifc->ifc_create)(ifc, name, len);
-	IF_CLONE_REMREF(ifc);
 	return (err);
 }
 
_at__at_ -156,7 +154,6 _at__at_ 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;
 		}
 	}
_at__at_ -172,7 +169,6 _at__at_ if_clone_destroy(const char *name)
 	err =  (*ifc->ifc_destroy)(ifc, ifp);
 
 done:
-	IF_CLONE_REMREF(ifc);
 	return (err);
 }
 
_at__at_ -353,6 +349,7 _at__at_ 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);
_at__at_ -375,7 +372,7 _at__at_ 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