>Number: 162789
>Category: kern
>Synopsis: [PATCH] if_clone may create multiple interfaces with the same
>name
>Confidential: no
>Severity: serious
>Priority: medium
>Responsible: freebsd-bugs
>State: open
>Quarter:
>Keywords:
>Date-Required:
>Class: sw-bug
>Submitter-Id: current-users
>Arrival-Date: Wed Nov 23 16:00:21 UTC 2011
>Closed-Date:
>Last-Modified:
>Originator: Daan Vreeken [PA4DAN]
>Release: FreeBSD 9.0-CURRENT amd64
>Organization:
Vitsch Electronics - http://Vitsch.nl/
>Environment:
System: FreeBSD Devel44.Vitsch.LAN 9.0-CURRENT FreeBSD 9.0-CURRENT #13
r223765M: Wed Nov 23 13:39:02 CET 2011 amd64
>Description:
The code in if_clone.c and if.c allows the creation of multiple
network interfaces with the same name, leading to interesting problems.
>How-To-Repeat:
example 1 (for code in if_clone.c):
# ifconfig bridge create
bridge0
# ifconfig bridge0 name bridge1
# ifconfig bridge create
bridge1
# ifconfig
em0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu
1500
...
em1: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu
1500
...
em2: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu
1500
...
lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> metric 0 mtu 16384
...
bridge1: flags=8802<BROADCAST,SIMPLEX,MULTICAST> metric 0 mtu 1500
ether 02:02:a6:0f:af:00
ether 02:02:a6:0f:af:01
nd6 options=29<PERFORMNUD,IFDISABLED,AUTO_LINKLOCAL>
id 00:00:00:00:00:00 priority 32768 hellotime 2 fwddelay 15
maxage 20 holdcnt 6 proto rstp maxaddr 100 timeout 1200
root id 00:00:00:00:00:00 priority 0 ifcost 0 port 0
[we now have 2x bridge1. ifconfig shows only one]
# ifconfig bridge1 destroy
# ifconfig bridge1 destroy
[... ifconfig hangs forever at this point ...]
example 2 (for code in if.c):
# ifconfig lo create
lo1
# ifconfig lo create
lo2
# ifconfig lo1 name foobar & ifconfig lo2 name foobar
# ifconfig
[very oftel we'll now have 2x foobar. ifconfig shows only one]
>Fix:
The attached patch does the following:
in if_clone.c :
Add locking around the code that finds a free unit number and then creates a
new interface in if_clone.c . In ifc_alloc_unit() it adds a check to make sure
there's no interface (possibly of another type) with the name that we're
going to create.
in if.c :
In the SIOCSIFNAME ioctl code, add locking around the code that checks if the
new interface name is unique and the code that actually renames the interface.
We can't use IFNET_WLOCK() here since the code paths may sleep.
In case the diff gets mangled in the mail, it can also be found here:
http://www.Vitsch.nl/pub_diffs
--- ifnet-naming-lock-2011-11-23.diff begins here ---
Index: sys/net/if_var.h
===================================================================
--- sys/net/if_var.h (revision 223765)
+++ sys/net/if_var.h (working copy)
@@ -790,6 +790,10 @@
sx_xunlock(&ifnet_sxlock); \
} while (0)
+#define IFNET_NAMING_LOCK() sx_xlock(&ifnet_sxlock);
+#define IFNET_NAMING_UNLOCK() sx_unlock(&ifnet_sxlock);
+#define IFNET_NAMING_ASSERT() sx_assert(&ifnet_sx, SA_XLOCKED);
+
/*
* To assert the ifnet lock, you must know not only whether it's for read or
* write, but also whether it was acquired with sleep support or not.
Index: sys/net/if.c
===================================================================
--- sys/net/if.c (revision 223765)
+++ sys/net/if.c (working copy)
@@ -2220,8 +2220,12 @@
return (error);
if (new_name[0] == '\0')
return (EINVAL);
- if (ifunit(new_name) != NULL)
+
+ IFNET_NAMING_LOCK();
+ if (ifunit(new_name) != NULL) {
+ IFNET_NAMING_UNLOCK();
return (EEXIST);
+ }
/*
* XXX: Locking. Nothing else seems to lock if_flags,
@@ -2260,6 +2264,7 @@
while (namelen != 0)
sdl->sdl_data[--namelen] = 0xff;
IFA_UNLOCK(ifa);
+ IFNET_NAMING_UNLOCK();
EVENTHANDLER_INVOKE(ifnet_arrival_event, ifp);
/* Announce the return of the interface. */
Index: sys/net/if_clone.c
===================================================================
--- sys/net/if_clone.c (revision 223765)
+++ sys/net/if_clone.c (working copy)
@@ -443,6 +443,8 @@
{
int wildcard, bytoff, bitoff;
int err = 0;
+ int found = 0;
+ char name[IFNAMSIZ];
IF_CLONE_LOCK(ifc);
@@ -451,7 +453,7 @@
/*
* Find a free unit if none was given.
*/
- if (wildcard) {
+ while ((wildcard) && (!found)) {
while ((bytoff < ifc->ifc_bmlen)
&& (ifc->ifc_units[bytoff] == 0xff))
bytoff++;
@@ -461,7 +463,24 @@
}
while ((ifc->ifc_units[bytoff] & (1 << bitoff)) != 0)
bitoff++;
+
+ /*
+ * we've found a free slot in the bitmap. now see if an
+ * interface with this exact name already exists
+ */
*unit = (bytoff << 3) + bitoff;
+ snprintf(name, IFNAMSIZ, "%s%d", ifc->ifc_name, *unit);
+ if (ifunit(name) != NULL) {
+ /* interface already exists.. try another one */
+ bitoff++;
+ if (bitoff == 8) {
+ bitoff = 0;
+ bytoff++;
+ }
+ continue;
+ }
+
+ found = 1;
}
if (*unit > ifc->ifc_maxunit) {
@@ -470,6 +489,13 @@
}
if (!wildcard) {
+ snprintf(name, IFNAMSIZ, "%s%d", ifc->ifc_name, *unit);
+ if (ifunit(name) != NULL) {
+ /* an interface with this exact name already exists */
+ err = EEXIST;
+ goto done;
+ }
+
bytoff = *unit >> 3;
bitoff = *unit - (bytoff << 3);
}
@@ -568,11 +594,16 @@
wildcard = (unit < 0);
+
+ IFNET_NAMING_LOCK();
err = ifc_alloc_unit(ifc, &unit);
- if (err != 0)
+ 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);
--- ifnet-naming-lock-2011-11-23.diff ends here ---
>Release-Note:
>Audit-Trail:
>Unformatted:
_______________________________________________
freebsd-bugs@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "freebsd-bugs-unsubscr...@freebsd.org"
