Re: if_clone.c allows creating multiple interfaces with the same name
On Tuesday, November 29, 2011 9:28:42 am Gleb Smirnoff wrote:
> Daan,
>
> On Tue, Nov 29, 2011 at 01:07:13AM +0100, Daan Vreeken wrote:
> D> Thanks for the looking into this and for your quick commit. I like your
> twist
> D> on the patch with the move from the unit bitmap to allocating unit numbers
> D> with alloc_unr(9).
> D>
> D> I do have two comments on the new code though.
> D> Before, in the !wildcard case, ifc_alloc_unit() would return ENOSPC when
> the
> D> user requested a unit number larger than ifc->ifc_maxunit. Now the function
> D> returns EEXIST in that case because alloc_unr_specific() returns -1 both
> D> when a number is already allocated and when the number exceeds it's limits.
> D> This leads to the somewhat confusing:
> D>
> D> # ifconfig bridge123456 create
> D> ifconfig: SIOCIFCREATE2: File exists
> D>
> D> Apart from that I'm a bit worried about the "/* XXXGL: yep, it's a unit
> leak
> D> */" part of your change. Once a unit number is leaked, there currently
> seems
> D> to be no way to ever get it back. In a worst case scenario, where the
> names of
> D> multiple interfaces in a row collides with the numbers alloc_unr()
> returns, an
> D> entire row of unit numbers could be leaked during the creation of a single
> D> cloned interface.
> D> We have a product where cloned interfaces come and go very frequently. I'd
> D> hate to run out of unit numbers after 'just a few' years of uptime ;-)
> D>
> D> I've created a new patch on top of your change that fixes both of these
> D> problems. Could you have a look at the attached diff?
>
> Thanks, I will work on applying it.
>
> D> > Considering the second part, that adds locking. Unfortunately, right now
> we
> D> > have numerous races in the network configuration ocde. Many
> SIOCSsomething
> D> > ioctls can race with each other producing unpredictable results and
> kernel
> D> > panics. So, running two ifconfig(8) in parallel is a bad idea today. :(
> D> > Your patch with IFNET_NAMING_LOCK() just plumbs one race case: a race
> D> > between two SIOCSIFNAME ioctls. And it doesn't plumb a race between
> D> > SIOCSIFNAME vs SIOCIFCREATE, because IFNET_NAMING_LOCK() is dropped after
> D> > unit allocation, but prior to interface attachement to global interface
> D> > list.
> D>
> D> Are you sure? With the patch in the PR, the relevant code in
> D> ifc_simple_create() would become :
> D>
> D>...
> D>IFNET_NAMING_LOCK();
> D>err = ifc_alloc_unit(ifc, &unit);
> D>if (err != 0) {
> D>IFNET_NAMING_UNLOCK();
> D>return (err);
> D>}
> D>
> D>err = ifcs->ifcs_create(ifc, unit, params);
> D>IFNET_NAMING_UNLOCK();
> D>if (err != 0) {
> D>ifc_free_unit(ifc, unit);
> D>return (err);
> D>}
> D>...
> D>
> D> The lock is acquired before the call to ifc_alloc_unit().
> D> In the non-error case (e.g. when creating an if_bridge interface) the call
> D> ends up calling bridge_clone_create(), which calls ether_ifattach(), which
> D> calls if_attach() -> if_attach_internal() where the ifp is added to the
> ifnet
> D> list.
> D> Only when that's done, the lock is dropped.
> D>
> D> Am I overlooking something obvious here?
>
> The code above isn't correct since it holds mutex when calling
> ifcs->ifcs_create().
> These methods may (they actually do) call malloc() with M_WAITOK.
>
> In general FreeBSD uses M_WAITOK on the configuration code paths, like
> any SIOCSsomething ioctl. So to do correct protection, you first need to
> allocate every kind of memory needed, then lock mutex, then run through
> configuration
> sequence, then release mutex and in case of error free all allocated memory.
> Sounds easy, but isn't in practice, especially when several network modules
> are involved.
>
> So I'm still thinking about...
>
> D> > From my point of view, we need a generic approach to ioctl() vs ioctl()
> D> > races, may be some global serializer of all re-configuration requests of
> D> > interfaces and addresses.
>
> ... but several developers have noted that this approach may have some hidden
> problems. When experimenting with new CARP, I have tried it on the
> carp_ioctl()
> without any problems. The idea is simple:
>
> static int serializer = 0;
> static struct mtx serializer_mtx;
> MTX_SYSINIT("sermtx", &serializer_mtx, "ioctl serializer mutex", MTX_DEF);
>
> int
> foo_ioctl()
> {
> mtx_lock(&serializer_mtx);
> if (serializer != 0)
> msleep(&serializer, &serializer_mtx, 0, "ioctl", 0);
> serializer = 1;
> mtx_unlock(&serializer_mtx);
>
> ... any code here, uncluding calls to different lower layers...
>
> mtx_lock(&serializer_mtx);
> serializer = 0;
> wakeup(&serializer);
> mtx_unlock(&serializer_mtx);
>
> return (error);
> }
>
> I have tried this for carp_ioctl() and it worked fine. You can try it for
> entire ifioctl() and all its descendants, but be a
Re: if_clone.c allows creating multiple interfaces with the same name
Hi Glebius,
On Tuesday 29 November 2011 15:28:42 Gleb Smirnoff wrote:
> Daan,
>
> On Tue, Nov 29, 2011 at 01:07:13AM +0100, Daan Vreeken wrote:
> D> Thanks for the looking into this and for your quick commit. I like your
> D> twist on the patch with the move from the unit bitmap to allocating unit
> D> numbers with alloc_unr(9).
> D>
> D> I do have two comments on the new code though.
> D> Before, in the !wildcard case, ifc_alloc_unit() would return ENOSPC when
> D> the user requested a unit number larger than ifc->ifc_maxunit. Now the
> D> function returns EEXIST in that case because alloc_unr_specific()
> D> returns -1 both when a number is already allocated and when the number
> D> exceeds it's limits. This leads to the somewhat confusing:
> D>
> D> # ifconfig bridge123456 create
> D> ifconfig: SIOCIFCREATE2: File exists
> D>
> D> Apart from that I'm a bit worried about the "/* XXXGL: yep, it's a unit
> D> leak */" part of your change. Once a unit number is leaked, there
> D> currently seems to be no way to ever get it back. In a worst case
> D> scenario, where the names of multiple interfaces in a row collides with
> D> the numbers alloc_unr() returns, an entire row of unit numbers could be
> D> leaked during the creation of a single cloned interface.
> D> We have a product where cloned interfaces come and go very frequently.
> D> I'd hate to run out of unit numbers after 'just a few' years of uptime
> D> ;-)
> D> I've created a new patch on top of your change that fixes both of these
> D> problems. Could you have a look at the attached diff?
>
> Thanks, I will work on applying it.
Great!
> D> > Considering the second part, that adds locking. Unfortunately, right
> D> > now we have numerous races in the network configuration ocde. Many
> D> > SIOCSsomething ioctls can race with each other producing unpredictable
> D> > results and kernel panics. So, running two ifconfig(8) in parallel is
> D> > a bad idea today. :(
> D> > Your patch with IFNET_NAMING_LOCK() just plumbs one race case: a race
> D> > between two SIOCSIFNAME ioctls. And it doesn't plumb a race between
> D> > SIOCSIFNAME vs SIOCIFCREATE, because IFNET_NAMING_LOCK() is dropped
> D> > after unit allocation, but prior to interface attachement to global
> D> > interface list.
> D>
> D> Are you sure? With the patch in the PR, the relevant code in
> D> ifc_simple_create() would become :
> D>
> D>...
> D>IFNET_NAMING_LOCK();
> D>err = ifc_alloc_unit(ifc, &unit);
> D>if (err != 0) {
> D>IFNET_NAMING_UNLOCK();
> D>return (err);
> D>}
> D>
> D>err = ifcs->ifcs_create(ifc, unit, params);
> D>IFNET_NAMING_UNLOCK();
> D>if (err != 0) {
> D>ifc_free_unit(ifc, unit);
> D>return (err);
> D>}
> D>...
> D>
> D> The lock is acquired before the call to ifc_alloc_unit().
> D> In the non-error case (e.g. when creating an if_bridge interface) the
> D> call ends up calling bridge_clone_create(), which calls
> D> ether_ifattach(), which calls if_attach() -> if_attach_internal() where
> D> the ifp is added to the ifnet list.
> D> Only when that's done, the lock is dropped.
> D>
> D> Am I overlooking something obvious here?
>
> The code above isn't correct since it holds mutex when calling
> ifcs->ifcs_create(). These methods may (they actually do) call malloc()
> with M_WAITOK.
I'd noticed the malloc() too (see my comment in the PR: "We can't use
IFNET_WLOCK() here since the code paths may sleep.")
The IFNET_WLOCK() macro is defined as:
#define IFNET_WLOCK() do {
\
sx_xlock(&ifnet_sxlock);
\
rw_wlock(&ifnet_rwlock);
\
} while (0)
Since rwlocks cannot be held while sleeping, I've added the IFNET_NAMING_
macros in the patch, defining them as :
#define IFNET_NAMING_LOCK() sx_xlock(&ifnet_sxlock);
#define IFNET_NAMING_UNLOCK() sx_unlock(&ifnet_sxlock);
This only does the first half of the work IFNET_WLOCK() normally does. My
understanding from reading the sx(9) manual is that holding an sx lock while
sleeping should be allowed.
(WITNESS and INVARIANTS didn't seem to disagree with me. :)
After acquiring the ifnet_sxlock, malloc() should be allowed to sleep while
allocating memory for the new interface. When it's done sleeping, the
interface can be added to the ifnet list in if_attach().
if_attach() will IFNET_WLOCK() which should recurse on the ifnet_sxlock and
acquire a lock on the ifnet_rwlock.
(I'm still confused as to why this wouldn't work.)
> In general FreeBSD uses M_WAITOK on the configuration code paths, like
> any SIOCSsomething ioctl. So to do correct protection, you first need to
> allocate every kind of memory needed, then lock mutex, then run through
> configuration sequence, then release mutex and in case of error free all
> allocated memory
Re: if_clone.c allows creating multiple interfaces with the same name
Daan,
On Tue, Nov 29, 2011 at 01:07:13AM +0100, Daan Vreeken wrote:
D> Thanks for the looking into this and for your quick commit. I like your twist
D> on the patch with the move from the unit bitmap to allocating unit numbers
D> with alloc_unr(9).
D>
D> I do have two comments on the new code though.
D> Before, in the !wildcard case, ifc_alloc_unit() would return ENOSPC when the
D> user requested a unit number larger than ifc->ifc_maxunit. Now the function
D> returns EEXIST in that case because alloc_unr_specific() returns -1 both
D> when a number is already allocated and when the number exceeds it's limits.
D> This leads to the somewhat confusing:
D>
D> # ifconfig bridge123456 create
D> ifconfig: SIOCIFCREATE2: File exists
D>
D> Apart from that I'm a bit worried about the "/* XXXGL: yep, it's a unit leak
D> */" part of your change. Once a unit number is leaked, there currently seems
D> to be no way to ever get it back. In a worst case scenario, where the names
of
D> multiple interfaces in a row collides with the numbers alloc_unr() returns,
an
D> entire row of unit numbers could be leaked during the creation of a single
D> cloned interface.
D> We have a product where cloned interfaces come and go very frequently. I'd
D> hate to run out of unit numbers after 'just a few' years of uptime ;-)
D>
D> I've created a new patch on top of your change that fixes both of these
D> problems. Could you have a look at the attached diff?
Thanks, I will work on applying it.
D> > Considering the second part, that adds locking. Unfortunately, right now we
D> > have numerous races in the network configuration ocde. Many SIOCSsomething
D> > ioctls can race with each other producing unpredictable results and kernel
D> > panics. So, running two ifconfig(8) in parallel is a bad idea today. :(
D> > Your patch with IFNET_NAMING_LOCK() just plumbs one race case: a race
D> > between two SIOCSIFNAME ioctls. And it doesn't plumb a race between
D> > SIOCSIFNAME vs SIOCIFCREATE, because IFNET_NAMING_LOCK() is dropped after
D> > unit allocation, but prior to interface attachement to global interface
D> > list.
D>
D> Are you sure? With the patch in the PR, the relevant code in
D> ifc_simple_create() would become :
D>
D> ...
D> IFNET_NAMING_LOCK();
D> err = ifc_alloc_unit(ifc, &unit);
D> if (err != 0) {
D> IFNET_NAMING_UNLOCK();
D> return (err);
D> }
D>
D> err = ifcs->ifcs_create(ifc, unit, params);
D> IFNET_NAMING_UNLOCK();
D> if (err != 0) {
D> ifc_free_unit(ifc, unit);
D> return (err);
D> }
D> ...
D>
D> The lock is acquired before the call to ifc_alloc_unit().
D> In the non-error case (e.g. when creating an if_bridge interface) the call
D> ends up calling bridge_clone_create(), which calls ether_ifattach(), which
D> calls if_attach() -> if_attach_internal() where the ifp is added to the ifnet
D> list.
D> Only when that's done, the lock is dropped.
D>
D> Am I overlooking something obvious here?
The code above isn't correct since it holds mutex when calling
ifcs->ifcs_create().
These methods may (they actually do) call malloc() with M_WAITOK.
In general FreeBSD uses M_WAITOK on the configuration code paths, like
any SIOCSsomething ioctl. So to do correct protection, you first need to
allocate every kind of memory needed, then lock mutex, then run through
configuration
sequence, then release mutex and in case of error free all allocated memory.
Sounds easy, but isn't in practice, especially when several network modules
are involved.
So I'm still thinking about...
D> > From my point of view, we need a generic approach to ioctl() vs ioctl()
D> > races, may be some global serializer of all re-configuration requests of
D> > interfaces and addresses.
... but several developers have noted that this approach may have some hidden
problems. When experimenting with new CARP, I have tried it on the carp_ioctl()
without any problems. The idea is simple:
static int serializer = 0;
static struct mtx serializer_mtx;
MTX_SYSINIT("sermtx", &serializer_mtx, "ioctl serializer mutex", MTX_DEF);
int
foo_ioctl()
{
mtx_lock(&serializer_mtx);
if (serializer != 0)
msleep(&serializer, &serializer_mtx, 0, "ioctl", 0);
serializer = 1;
mtx_unlock(&serializer_mtx);
... any code here, uncluding calls to different lower layers...
mtx_lock(&serializer_mtx);
serializer = 0;
wakeup(&serializer);
mtx_unlock(&serializer_mtx);
return (error);
}
I have tried this for carp_ioctl() and it worked fine. You can try it for
entire ifioctl() and all its descendants, but be aware of hidden problems :)
--
Totus tuus, Glebius.
___
freebsd-current@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
Re: if_clone.c allows creating multiple interfaces with the same name
Hi Glebius,
On Friday 25 November 2011 15:32:58 Gleb Smirnoff wrote:
> 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.
Thanks for the looking into this and for your quick commit. I like your twist
on the patch with the move from the unit bitmap to allocating unit numbers
with alloc_unr(9).
I do have two comments on the new code though.
Before, in the !wildcard case, ifc_alloc_unit() would return ENOSPC when the
user requested a unit number larger than ifc->ifc_maxunit. Now the function
returns EEXIST in that case because alloc_unr_specific() returns -1 both
when a number is already allocated and when the number exceeds it's limits.
This leads to the somewhat confusing:
# ifconfig bridge123456 create
ifconfig: SIOCIFCREATE2: File exists
Apart from that I'm a bit worried about the "/* XXXGL: yep, it's a unit leak
*/" part of your change. Once a unit number is leaked, there currently seems
to be no way to ever get it back. In a worst case scenario, where the names of
multiple interfaces in a row collides with the numbers alloc_unr() returns, an
entire row of unit numbers could be leaked during the creation of a single
cloned interface.
We have a product where cloned interfaces come and go very frequently. I'd
hate to run out of unit numbers after 'just a few' years of uptime ;-)
I've created a new patch on top of your change that fixes both of these
problems. Could you have a look at the attached diff?
In case the attachment doesn't survive the list, it can also be found here:
http://www.vitsch.nl/pub-diffs/if_clone-ENOSPC-and-unr-leak-fix-2011-11-29.diff
> 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.
Are you sure? With the patch in the PR, the relevant code in
ifc_simple_create() would become :
...
IFNET_NAMING_LOCK();
err = ifc_alloc_unit(ifc, &unit);
if (err != 0) {
IFNET_NAMING_UNLOCK();
return (err);
}
err = ifcs->ifcs_create(ifc, unit, params);
IFNET_NAMING_UNLOCK();
if (err != 0) {
ifc_free_unit(ifc, unit);
return (err);
}
...
The lock is acquired before the call to ifc_alloc_unit().
In the non-error case (e.g. when creating an if_bridge interface) the call
ends up calling bridge_clone_create(), which calls ether_ifattach(), which
calls if_attach() -> if_attach_internal() where the ifp is added to the ifnet
list.
Only when that's done, the lock is dropped.
Am I overlooking something obvious here?
> 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.
Thanks,
--
Daan Vreeken
Vitsch Electronics
http://Vitsch.nl
tel: +31-(0)40-7113051 / +31-(0)6-46210825
KvK nr: 17174380
Index: sys/net/if_clone.c
===
--- sys/net/if_clone.c (revision 228109)
+++ sys/net/if_clone.c (working copy)
@@ -436,30 +436,49 @@
}
int
-ifc_alloc_unit(struct if_clone *ifc, int *unit)
+ifc_alloc_unit(struct if_clone *ifc, int *unit_nr)
{
char name[IFNAMSIZ];
- int wildcard;
+ int ret, unit, wildcard;
- wildcard = (*unit < 0);
+ unit = *unit_nr;
+ wildcard = (unit < 0);
retry:
- if (wildcard) {
- *unit = alloc_unr(ifc->ifc_unrhdr);
- if (*unit == -1)
+ if (unit > ifc->ifc_maxunit)
+ return (ENOSPC);
+
+ if (unit < 0) {
+ /* first, try to allocate a unit number automatically */
+ unit = alloc_unr(ifc->ifc_unrhdr);
+ if (unit == -1)
return (ENOSPC);
} else {
- *unit = alloc_unr_specific(ifc->ifc_unrhdr, *unit);
- if (*unit == -
Re: if_clone.c allows creating multiple interfaces with the same name
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.
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_C
Re: if_clone.c allows creating multiple interfaces with the same name
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) B
Re: if_clone.c allows creating multiple interfaces with the same name
On Thu, Nov 24, 2011 at 09:28:51AM +0100, Daan Vreeken wrote: D> Hi All, D> 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'll try to handle this. -- Totus tuus, Glebius. ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
if_clone.c allows creating multiple interfaces with the same name
Hi All, Recently I've discovered a bug in if_clone.c and if.c where the code allows multiple interfaces to be created with exactly the same name (which leads to all sorts of other interesting problems). I've submitted a PR about this with patches, which can be found here : http://www.freebsd.org/cgi/query-pr.cgi?pr=162789 Could anyone take a look at it? Regards, -- Daan Vreeken Vitsch Electronics http://Vitsch.nl tel: +31-(0)40-7113051 / +31-(0)6-46210825 KvK nr: 17174380 ___ freebsd-current@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-current To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"
