From owner-freebsd-bugs@FreeBSD.ORG Wed Nov 23 16:00:22 2011 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9D021106566B for ; Wed, 23 Nov 2011 16:00:22 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 659618FC17 for ; Wed, 23 Nov 2011 16:00:22 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.4/8.14.4) with ESMTP id pANG0MH0060975 for ; Wed, 23 Nov 2011 16:00:22 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.4/8.14.4/Submit) id pANG0Mxh060974; Wed, 23 Nov 2011 16:00:22 GMT (envelope-from gnats) Resent-Date: Wed, 23 Nov 2011 16:00:22 GMT Resent-Message-Id: <201111231600.pANG0Mxh060974@freefall.freebsd.org> Resent-From: FreeBSD-gnats-submit@FreeBSD.org (GNATS Filer) Resent-To: freebsd-bugs@FreeBSD.org Resent-Reply-To: FreeBSD-gnats-submit@FreeBSD.org, "Daan Vreeken [PA4DAN]" Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B58D91065679 for ; Wed, 23 Nov 2011 15:58:40 +0000 (UTC) (envelope-from Daan@Vitsch.nl) Received: from Bliksem.VEHosting.nl (Bliksem6.VEHosting.nl [IPv6:2001:1af8:2100:b020::141]) by mx1.freebsd.org (Postfix) with ESMTP id 0A0BF8FC14 for ; Wed, 23 Nov 2011 15:58:39 +0000 (UTC) Received: from vitsch.nl (localhost [127.0.0.1]) by Bliksem.VEHosting.nl (8.13.8/8.13.8) with SMTP id pANFwblX022777; Wed, 23 Nov 2011 16:58:37 +0100 (CET) (envelope-from Daan@Vitsch.nl) Received: from Prakkezator.VEHosting.nl (localhost [127.0.0.1]) by Prakkezator.VEHosting.nl (8.14.2/8.14.2) with ESMTP id pANFqjf0040557; Wed, 23 Nov 2011 16:52:45 +0100 (CET) (envelope-from pa4dan@Prakkezator.VEHosting.nl) Received: (from pa4dan@localhost) by Prakkezator.VEHosting.nl (8.14.2/8.14.2/Submit) id pANFqj68040556; Wed, 23 Nov 2011 16:52:45 +0100 (CET) (envelope-from pa4dan) Message-Id: <201111231552.pANFqj68040556@Prakkezator.VEHosting.nl> Date: Wed, 23 Nov 2011 16:52:45 +0100 (CET) From: "Daan Vreeken [PA4DAN]" To: FreeBSD-gnats-submit@FreeBSD.org X-Send-Pr-Version: 3.113 Cc: Daan@Vitsch.nl Subject: kern/162789: [PATCH] if_clone may create multiple interfaces with the same name X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: "Daan Vreeken \[PA4DAN\]" List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 23 Nov 2011 16:00:22 -0000 >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 metric 0 mtu 1500 ... em1: flags=8843 metric 0 mtu 1500 ... em2: flags=8843 metric 0 mtu 1500 ... lo0: flags=8049 metric 0 mtu 16384 ... bridge1: flags=8802 metric 0 mtu 1500 ether 02:02:a6:0f:af:00 ether 02:02:a6:0f:af:01 nd6 options=29 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: