Re: panic: ifc_free_unit: bit is already cleared

On Mon, Oct 10, 2005 at 03:22:08PM +1300, Andrew Thompson wrote:
> On Mon, Oct 10, 2005 at 03:28:49AM +0400, Yar Tikhiy wrote:
> > On Thu, Oct 06, 2005 at 10:09:50AM +1300, Andrew Thompson wrote:
> > > 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.
> > > 
> > > I dont think this is the problem. IF_CLONE_REMREF(ifc) is freeing
> > > ifc->ifc_units in if_clone_detach(). It look like the ref counting isnt
> > > working quite right.
> > 
> > FWIW, I tried to look at the $subject problem since I had had it
> > before, but just got a different panic:
> > 
> >         Memory modified after free 0xc140b000(4092) val=deadc0dc _at_ 0xc140b000
> >         panic: Most recently used by clone
> > 
> > The clone code seems to have decremented something (refcount?) twice
> > after freeing the memory chunk.
> 
> I have been testing this patch and I think it fixes all the problems
> discussed.
> 
> It changes refcounting to count the number of cloned interfaces so
> ifc_units is only freed when its safe. A new function has been added to
> decrement this when a simple cloner module is unloaded. The cloner is
> still detached first to prevent the race.
> 
> In most cases the change is as simple as:
>                 while ((sc = LIST_FIRST(&gre_softc_list)) != NULL) {
>                         LIST_REMOVE(sc, sc_list);
>                         mtx_unlock(&gre_mtx);
> +                       ifc_simple_free(&gre_cloner, GRE2IFP(sc));
>                         gre_destroy(sc);
>                         mtx_lock(&gre_mtx);
>                 }

I don't see any reason why you can't just replace the specific destroy
calls with calls to ifc_simple_destroy().  That would avoid expanding
the API.

-- Brooks

-- 
Any statement of the form "X is the one, true Y" is FALSE.
PGP fingerprint 655D 519C 26A7 82E7 2529  9BF0 5D8E 8BE9 F238 1AD4