Date: Wed, 23 Nov 2011 16:52:45 +0100 (CET) From: "Daan Vreeken [PA4DAN]" <Daan@Vitsch.nl> To: FreeBSD-gnats-submit@FreeBSD.org Cc: Daan@Vitsch.nl Subject: kern/162789: [PATCH] if_clone may create multiple interfaces with the same name Message-ID: <201111231552.pANFqj68040556@Prakkezator.VEHosting.nl> Resent-Message-ID: <201111231600.pANG0Mxh060974@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
>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:
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201111231552.pANFqj68040556>