Date: Fri, 25 Nov 2011 18:32:58 +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: <20111125143257.GR96616@FreeBSD.org> In-Reply-To: <20111125131935.GP96616@FreeBSD.org> References: <201111240928.51162.Daan@vitsch.nl> <20111125131935.GP96616@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--VUDLurXRWRKrGuMn Content-Type: text/plain; charset=koi8-r Content-Disposition: inline On Fri, Nov 25, 2011 at 05:19:35PM +0400, Gleb Smirnoff wrote: T> On Thu, Nov 24, 2011 at 09:28:51AM +0100, Daan Vreeken wrote: T> D> Recently I've discovered a bug in if_clone.c and if.c where the code allows T> D> multiple interfaces to be created with exactly the same name (which leads to T> D> all sorts of other interesting problems). T> D> I've submitted a PR about this with patches, which can be found here : T> D> T> D> http://www.freebsd.org/cgi/query-pr.cgi?pr=162789 T> D> T> D> Could anyone take a look at it? T> T> I decided to simply if_clone code utilizing generic unit allocator. Patch T> atteched. Now I'll try to merge it with your ideas. Here is if_cloner patched with additional ifunit() check, as you suggested. Please review my patch and test it, and then we can commit it. Considering the second part, that adds locking. Unfortunately, right now we have numerous races in the network configuration ocde. Many SIOCSsomething ioctls can race with each other producing unpredictable results and kernel panics. So, running two ifconfig(8) in parallel is a bad idea today. :( Your patch with IFNET_NAMING_LOCK() just plumbs one race case: a race between two SIOCSIFNAME ioctls. And it doesn't plumb a race between SIOCSIFNAME vs SIOCIFCREATE, because IFNET_NAMING_LOCK() is dropped after unit allocation, but prior to interface attachement to global interface list. >From my point of view, we need a generic approach to ioctl() vs ioctl() races, may be some global serializer of all re-configuration requests of interfaces and addresses. -- Totus tuus, Glebius. --VUDLurXRWRKrGuMn Content-Type: text/x-diff; charset=koi8-r Content-Disposition: attachment; filename="if_clone_alloc_unr2.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,40 @@ int ifc_alloc_unit(struct if_clone *ifc, int *unit) { - int wildcard, bytoff, bitoff; - int err = 0; + char name[IFNAMSIZ]; + int wildcard; - IF_CLONE_LOCK(ifc); - - bytoff = bitoff = 0; wildcard = (*unit < 0); - /* - * Find a free unit if none was given. - */ +retry: 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; + *unit = alloc_unr(ifc->ifc_unrhdr); + if (*unit == -1) + return (ENOSPC); + } else { + *unit = alloc_unr_specific(ifc->ifc_unrhdr, *unit); + if (*unit == -1) + return (EEXIST); } - if (*unit > ifc->ifc_maxunit) { - err = ENOSPC; - goto done; + snprintf(name, IFNAMSIZ, "%s%d", ifc->ifc_name, *unit); + if (ifunit(name) != NULL) { + if (wildcard) + goto retry; /* XXXGL: yep, it's a unit leak */ + else + return (EEXIST); } - 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); --VUDLurXRWRKrGuMn--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20111125143257.GR96616>