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.
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);
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[email protected]"
