Date: Fri, 25 Nov 2011 17:19:35 +0400 From: Gleb Smirnoff <glebius@FreeBSD.org> To: Daan Vreeken <Daan@vitsch.nl> Cc: FreeBSD Current <freebsd-current@FreeBSD.org> Subject: Re: if_clone.c allows creating multiple interfaces with the same name Message-ID: <20111125131935.GP96616@FreeBSD.org> In-Reply-To: <201111240928.51162.Daan@vitsch.nl> References: <201111240928.51162.Daan@vitsch.nl>
next in thread | previous in thread | raw e-mail | index | archive | help
--HG+GLK89HZ1zG0kk Content-Type: text/plain; charset=koi8-r Content-Disposition: inline On Thu, Nov 24, 2011 at 09:28:51AM +0100, Daan Vreeken wrote: D> Recently I've discovered a bug in if_clone.c and if.c where the code allows D> multiple interfaces to be created with exactly the same name (which leads to D> all sorts of other interesting problems). D> I've submitted a PR about this with patches, which can be found here : D> D> http://www.freebsd.org/cgi/query-pr.cgi?pr=162789 D> D> Could anyone take a look at it? I decided to simply if_clone code utilizing generic unit allocator. Patch atteched. Now I'll try to merge it with your ideas. -- Totus tuus, Glebius. --HG+GLK89HZ1zG0kk Content-Type: text/x-diff; charset=koi8-r Content-Disposition: attachment; filename="if_clone_alloc_unr.diff" Index: if_clone.c =================================================================== --- if_clone.c (revision 227964) +++ if_clone.c (working copy) @@ -282,33 +282,34 @@ /* * Register a network interface cloner. */ -void +int if_clone_attach(struct if_clone *ifc) { - int len, maxclone; + struct if_clone *ifc1; - /* - * Compute bitmap size and allocate it. - */ - maxclone = ifc->ifc_maxunit + 1; - len = maxclone >> 3; - if ((len << 3) < maxclone) - len++; - ifc->ifc_units = malloc(len, M_CLONE, M_WAITOK | M_ZERO); - ifc->ifc_bmlen = len; + KASSERT(ifc->ifc_name != NULL, ("%s: no name\n", __func__)); + IF_CLONE_LOCK_INIT(ifc); IF_CLONE_ADDREF(ifc); + ifc->ifc_unrhdr = new_unrhdr(0, ifc->ifc_maxunit, &ifc->ifc_mtx); + LIST_INIT(&ifc->ifc_iflist); IF_CLONERS_LOCK(); + LIST_FOREACH(ifc1, &V_if_cloners, ifc_list) + if (strcmp(ifc->ifc_name, ifc1->ifc_name) == 0) { + IF_CLONERS_UNLOCK(); + IF_CLONE_REMREF(ifc); + return (EEXIST); + } LIST_INSERT_HEAD(&V_if_cloners, ifc, ifc_list); V_if_cloners_count++; IF_CLONERS_UNLOCK(); - LIST_INIT(&ifc->ifc_iflist); - if (ifc->ifc_attach != NULL) (*ifc->ifc_attach)(ifc); EVENTHANDLER_INVOKE(if_clone_event, ifc); + + return (0); } /* @@ -338,16 +339,12 @@ static void if_clone_free(struct if_clone *ifc) { - for (int bytoff = 0; bytoff < ifc->ifc_bmlen; bytoff++) { - KASSERT(ifc->ifc_units[bytoff] == 0x00, - ("ifc_units[%d] is not empty", bytoff)); - } KASSERT(LIST_EMPTY(&ifc->ifc_iflist), ("%s: ifc_iflist not empty", __func__)); IF_CLONE_LOCK_DESTROY(ifc); - free(ifc->ifc_units, M_CLONE); + delete_unrhdr(ifc->ifc_unrhdr); } /* @@ -441,73 +438,32 @@ int ifc_alloc_unit(struct if_clone *ifc, int *unit) { - int wildcard, bytoff, bitoff; - int err = 0; + int error = 0; - IF_CLONE_LOCK(ifc); - - bytoff = bitoff = 0; - wildcard = (*unit < 0); - /* - * Find a free unit if none was given. - */ - if (wildcard) { - while ((bytoff < ifc->ifc_bmlen) - && (ifc->ifc_units[bytoff] == 0xff)) - bytoff++; - if (bytoff >= ifc->ifc_bmlen) { - err = ENOSPC; - goto done; - } - while ((ifc->ifc_units[bytoff] & (1 << bitoff)) != 0) - bitoff++; - *unit = (bytoff << 3) + bitoff; + if (*unit < 0) { /* Wildcard. */ + *unit = alloc_unr(ifc->ifc_unrhdr); + if (*unit == -1) + error = ENOSPC; + } else { + *unit = alloc_unr_specific(ifc->ifc_unrhdr, *unit); + if (*unit == -1) + error = EEXIST; } - if (*unit > ifc->ifc_maxunit) { - err = ENOSPC; - goto done; - } + if (error) + return (error); - if (!wildcard) { - bytoff = *unit >> 3; - bitoff = *unit - (bytoff << 3); - } + IF_CLONE_ADDREF(ifc); - if((ifc->ifc_units[bytoff] & (1 << bitoff)) != 0) { - err = EEXIST; - goto done; - } - /* - * 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); - return (err); + return (0); } void ifc_free_unit(struct if_clone *ifc, int unit) { - int bytoff, bitoff; - - /* - * Compute offset in the bitmap and deallocate the unit. - */ - bytoff = unit >> 3; - bitoff = unit - (bytoff << 3); - - IF_CLONE_LOCK(ifc); - KASSERT((ifc->ifc_units[bytoff] & (1 << bitoff)) != 0, - ("%s: bit is already cleared", __func__)); - ifc->ifc_units[bytoff] &= ~(1 << bitoff); - IF_CLONE_REMREF_LOCKED(ifc); /* releases lock */ + free_unr(ifc->ifc_unrhdr, unit); + IF_CLONE_REMREF(ifc); } void Index: if_clone.h =================================================================== --- if_clone.h (revision 227964) +++ if_clone.h (working copy) @@ -37,7 +37,15 @@ #define IFC_CLONE_INITIALIZER(name, data, maxunit, \ attach, match, create, destroy) \ - { { 0 }, name, maxunit, NULL, 0, data, attach, match, create, destroy } + { \ + .ifc_name = name, \ + .ifc_maxunit = maxunit, \ + .ifc_data = data, \ + .ifc_attach = attach, \ + .ifc_match = match, \ + .ifc_create = create, \ + .ifc_destroy = destroy, \ + } /* * Structure describing a `cloning' interface. @@ -52,10 +60,7 @@ LIST_ENTRY(if_clone) ifc_list; /* (e) On list of cloners */ const char *ifc_name; /* (c) Name of device, e.g. `gif' */ int ifc_maxunit; /* (c) Maximum unit number */ - unsigned char *ifc_units; /* (i) Bitmap to handle units. */ - /* Considered private, access */ - /* via ifc_(alloc|free)_unit(). */ - int ifc_bmlen; /* (c) Bitmap length. */ + struct unrhdr *ifc_unrhdr; /* (c) alloc_unr(9) header */ void *ifc_data; /* (*) Data for ifc_* functions. */ /* (c) Driver specific cloning functions. Called with no locks held. */ @@ -65,12 +70,12 @@ int (*ifc_destroy)(struct if_clone *, struct ifnet *); long ifc_refcnt; /* (i) Refrence count. */ - struct mtx ifc_mtx; /* Muted to protect members. */ + struct mtx ifc_mtx; /* Mutex to protect members. */ LIST_HEAD(, ifnet) ifc_iflist; /* (i) List of cloned interfaces */ }; void if_clone_init(void); -void if_clone_attach(struct if_clone *); +int if_clone_attach(struct if_clone *); void if_clone_detach(struct if_clone *); void vnet_if_clone_init(void); --HG+GLK89HZ1zG0kk--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111125131935.GP96616>