Date: Mon, 10 Oct 2005 15:22:08 +1300 From: Andrew Thompson <thompsa@freebsd.org> To: Yar Tikhiy <yar@comp.chem.msu.su> 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: <20051010022208.GA97249@heff.fud.org.nz> In-Reply-To: <20051009232849.GA27349@comp.chem.msu.su> References: <20051005024903.GA72743@heff.fud.org.nz> <20051005203639.GA20552@garage.freebsd.pl> <20051005205515.GA30350@odin.ac.hmc.edu> <20051005210950.GB75848@heff.fud.org.nz> <20051009232849.GA27349@comp.chem.msu.su>
next in thread | previous in thread | raw e-mail | index | archive | help
--J/dobhs11T7y2rNN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 @ 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); } Andrew --J/dobhs11T7y2rNN Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="ifclone.diff" Index: contrib/pf/net/if_pflog.c =================================================================== RCS file: /home/ncvs/src/sys/contrib/pf/net/if_pflog.c,v retrieving revision 1.15 diff -u -p -r1.15 if_pflog.c --- contrib/pf/net/if_pflog.c 9 Aug 2005 11:59:02 -0000 1.15 +++ contrib/pf/net/if_pflog.c 10 Oct 2005 01:56:26 -0000 @@ -360,6 +360,7 @@ static int pflog_modevent(module_t mod, int type, void *data) { int error = 0; + struct ifnet *ifp; switch (type) { case MOD_LOAD: @@ -369,8 +370,11 @@ pflog_modevent(module_t mod, int type, v case MOD_UNLOAD: if_clone_detach(&pflog_cloner); - while (!LIST_EMPTY(&pflog_list)) - pflog_clone_destroy(SCP2IFP(LIST_FIRST(&pflog_list))); + while (!LIST_EMPTY(&pflog_list)) { + ifp = SCP2IFP(LIST_FIRST(&pflog_list)); + ifc_simple_free(&pflog_cloner, ifp); + pflog_clone_destroy(ifp); + } break; default: Index: contrib/pf/net/if_pfsync.c =================================================================== RCS file: /home/ncvs/src/sys/contrib/pf/net/if_pfsync.c,v retrieving revision 1.23 diff -u -p -r1.23 if_pfsync.c --- contrib/pf/net/if_pfsync.c 11 Sep 2005 11:55:39 -0000 1.23 +++ contrib/pf/net/if_pfsync.c 10 Oct 2005 01:56:32 -0000 @@ -1842,6 +1842,7 @@ static int pfsync_modevent(module_t mod, int type, void *data) { int error = 0; + struct ifnet *ifp; switch (type) { case MOD_LOAD: @@ -1851,9 +1852,11 @@ pfsync_modevent(module_t mod, int type, case MOD_UNLOAD: if_clone_detach(&pfsync_cloner); - while (!LIST_EMPTY(&pfsync_list)) - pfsync_clone_destroy( - SCP2IFP(LIST_FIRST(&pfsync_list))); + while (!LIST_EMPTY(&pfsync_list)) { + ifp = SCP2IFP(LIST_FIRST(&pfsync_list)); + ifc_simple_free(&pfsync_cloner, ifp); + pfsync_clone_destroy(ifp); + } break; default: Index: net/if_bridge.c =================================================================== RCS file: /home/ncvs/src/sys/net/if_bridge.c,v retrieving revision 1.23 diff -u -p -r1.23 if_bridge.c --- net/if_bridge.c 2 Oct 2005 19:15:56 -0000 1.23 +++ net/if_bridge.c 10 Oct 2005 01:56:43 -0000 @@ -356,8 +356,11 @@ bridge_modevent(module_t mod, int type, break; case MOD_UNLOAD: if_clone_detach(&bridge_cloner); - while (!LIST_EMPTY(&bridge_list)) + while (!LIST_EMPTY(&bridge_list)) { + ifc_simple_free(&bridge_cloner, + LIST_FIRST(&bridge_list)->sc_ifp); bridge_clone_destroy(LIST_FIRST(&bridge_list)->sc_ifp); + } uma_zdestroy(bridge_rtnode_zone); bridge_input_p = NULL; bridge_output_p = NULL; Index: net/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 --- net/if_clone.c 24 Feb 2005 13:14:41 -0000 1.6 +++ net/if_clone.c 10 Oct 2005 01:09:08 -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); } @@ -352,7 +348,10 @@ ifc_alloc_unit(struct if_clone *ifc, int /* * Allocate the unit in the bitmap. */ + KASSERT((ifc->ifc_units[bytoff] & (1 << bitoff)) == 0, + ("%s: bit is already set", __func__)); ifc->ifc_units[bytoff] |= (1 << bitoff); + IF_CLONE_ADDREF_LOCKED(ifc); done: IF_CLONE_UNLOCK(ifc); @@ -375,7 +374,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 @@ -477,6 +476,22 @@ ifc_simple_destroy(struct if_clone *ifc, ifcs->ifcs_destroy(ifp); + ifc_simple_free(ifc, ifp); + + return (0); +} + +int +ifc_simple_free(struct if_clone *ifc, struct ifnet *ifp) +{ + int unit; + struct ifc_simple_data *ifcs = ifc->ifc_data; + + unit = ifp->if_dunit; + + if (unit < ifcs->ifcs_minifs) + return (EINVAL); + ifc_free_unit(ifc, unit); return (0); Index: net/if_clone.h =================================================================== RCS file: /home/ncvs/src/sys/net/if_clone.h,v retrieving revision 1.2 diff -u -p -r1.2 if_clone.h --- net/if_clone.h 7 Jan 2005 01:45:34 -0000 1.2 +++ net/if_clone.h 9 Oct 2005 23:50:05 -0000 @@ -107,6 +107,7 @@ void ifc_simple_attach(struct if_clone * int ifc_simple_match(struct if_clone *, const char *); int ifc_simple_create(struct if_clone *, char *, size_t); int ifc_simple_destroy(struct if_clone *, struct ifnet *); +int ifc_simple_free(struct if_clone *, struct ifnet *); #endif /* _KERNEL */ Index: net/if_disc.c =================================================================== RCS file: /home/ncvs/src/sys/net/if_disc.c,v retrieving revision 1.48 diff -u -p -r1.48 if_disc.c --- net/if_disc.c 26 Jun 2005 18:11:10 -0000 1.48 +++ net/if_disc.c 10 Oct 2005 01:57:12 -0000 @@ -152,6 +152,7 @@ disc_modevent(module_t mod, int type, vo while ((sc = LIST_FIRST(&disc_softc_list)) != NULL) { LIST_REMOVE(sc, sc_list); mtx_unlock(&disc_mtx); + ifc_simple_free(&disc_cloner, sc->sc_ifp); disc_destroy(sc); mtx_lock(&disc_mtx); } Index: net/if_faith.c =================================================================== RCS file: /home/ncvs/src/sys/net/if_faith.c,v retrieving revision 1.37 diff -u -p -r1.37 if_faith.c --- net/if_faith.c 9 Aug 2005 10:19:58 -0000 1.37 +++ net/if_faith.c 10 Oct 2005 01:57:18 -0000 @@ -139,6 +139,7 @@ faithmodevent(mod, type, data) while ((sc = LIST_FIRST(&faith_softc_list)) != NULL) { LIST_REMOVE(sc, sc_list); mtx_unlock(&faith_mtx); + ifc_simple_free(&faith_cloner, sc->sc_ifp); faith_destroy(sc); mtx_lock(&faith_mtx); } Index: net/if_gif.c =================================================================== RCS file: /home/ncvs/src/sys/net/if_gif.c,v retrieving revision 1.54 diff -u -p -r1.54 if_gif.c --- net/if_gif.c 9 Aug 2005 10:19:58 -0000 1.54 +++ net/if_gif.c 10 Oct 2005 01:57:23 -0000 @@ -252,6 +252,7 @@ gifmodevent(mod, type, data) while ((sc = LIST_FIRST(&gif_softc_list)) != NULL) { LIST_REMOVE(sc, gif_list); mtx_unlock(&gif_mtx); + ifc_simple_free(&gif_cloner, GIF2IFP(sc)); gif_destroy(sc); mtx_lock(&gif_mtx); } Index: net/if_gre.c =================================================================== RCS file: /home/ncvs/src/sys/net/if_gre.c,v retrieving revision 1.34 diff -u -p -r1.34 if_gre.c --- net/if_gre.c 9 Aug 2005 10:19:58 -0000 1.34 +++ net/if_gre.c 10 Oct 2005 01:57:31 -0000 @@ -793,6 +793,7 @@ gremodevent(module_t mod, int type, void 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); } Index: net/if_ppp.c =================================================================== RCS file: /home/ncvs/src/sys/net/if_ppp.c,v retrieving revision 1.108 diff -u -p -r1.108 if_ppp.c --- net/if_ppp.c 19 Sep 2005 22:27:07 -0000 1.108 +++ net/if_ppp.c 10 Oct 2005 01:57:38 -0000 @@ -298,6 +298,7 @@ ppp_modevent(module_t mod, int type, voi while ((sc = LIST_FIRST(&ppp_softc_list)) != NULL) { LIST_REMOVE(sc, sc_list); PPP_LIST_UNLOCK(); + ifc_simple_free(&ppp_cloner, PPP2IFP(sc)); ppp_destroy(sc); PPP_LIST_LOCK(); } Index: netinet/ip_carp.c =================================================================== RCS file: /home/ncvs/src/sys/netinet/ip_carp.c,v retrieving revision 1.31 diff -u -p -r1.31 ip_carp.c --- netinet/ip_carp.c 9 Sep 2005 08:41:39 -0000 1.31 +++ netinet/ip_carp.c 10 Oct 2005 01:57:52 -0000 @@ -2133,6 +2133,7 @@ static int carp_modevent(module_t mod, int type, void *data) { int error = 0; + struct ifnet *ifp; switch (type) { case MOD_LOAD: @@ -2143,8 +2144,11 @@ carp_modevent(module_t mod, int type, vo case MOD_UNLOAD: if_clone_detach(&carp_cloner); - while (!LIST_EMPTY(&carpif_list)) - carp_clone_destroy(SC2IFP(LIST_FIRST(&carpif_list))); + while (!LIST_EMPTY(&carpif_list)) { + ifp = SC2IFP(LIST_FIRST(&carpif_list)); + ifc_simple_free(&carp_cloner, ifp); + carp_clone_destroy(ifp); + } mtx_destroy(&carp_mtx); break; --J/dobhs11T7y2rNN--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20051010022208.GA97249>