From Daan@Vitsch.nl Wed Nov 23 15:58:40 2011 Return-Path: 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]" Reply-To: "Daan Vreeken [PA4DAN]" To: FreeBSD-gnats-submit@freebsd.org Cc: Daan@Vitsch.nl Subject: [PATCH] if_clone may create multiple interfaces with the same name X-Send-Pr-Version: 3.113 X-GNATS-Notify: >Number: 162789 >Category: kern >Synopsis: [PATCH] if_clone may create multiple interfaces with the same name >Confidential: no >Severity: serious >Priority: medium >Responsible: glebius >State: patched >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: Mon Nov 28 14:50:08 UTC 2011 >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: State-Changed-From-To: open->patched State-Changed-By: glebius State-Changed-When: Mon Nov 28 14:45:33 UTC 2011 State-Changed-Why: Partially fixed in head. A check in if_cloner against ifunit() added. Locking isn't added, since it isn't sufficient. Responsible-Changed-From-To: freebsd-bugs->glebius Responsible-Changed-By: glebius Responsible-Changed-When: Mon Nov 28 14:45:33 UTC 2011 Responsible-Changed-Why: Partially fixed in head. A check in if_cloner against ifunit() added. Locking isn't added, since it isn't sufficient. http://www.freebsd.org/cgi/query-pr.cgi?pr=162789 From: dfilter@FreeBSD.ORG (dfilter service) To: bug-followup@FreeBSD.org Cc: Subject: Re: kern/162789: commit references a PR Date: Mon, 28 Nov 2011 14:45:16 +0000 (UTC) Author: glebius Date: Mon Nov 28 14:44:59 2011 New Revision: 228071 URL: http://svn.freebsd.org/changeset/base/228071 Log: - Use generic alloc_unr(9) allocator for if_clone, instead of hand-made. - When registering new cloner, check whether a cloner with same name already exist. - When allocating unit, also check with help of ifunit() whether such interface already exist or not. [1] PR: kern/162789 [1] Modified: head/sys/net/if_clone.c head/sys/net/if_clone.h Modified: head/sys/net/if_clone.c ============================================================================== --- head/sys/net/if_clone.c Mon Nov 28 14:39:56 2011 (r228070) +++ head/sys/net/if_clone.c Mon Nov 28 14:44:59 2011 (r228071) @@ -282,33 +282,34 @@ if_clone_destroyif(struct if_clone *ifc, /* * Register a network interface cloner. */ -void +int if_clone_attach(struct if_clone *ifc) { - int len, maxclone; + struct if_clone *ifc1; + + KASSERT(ifc->ifc_name != NULL, ("%s: no name\n", __func__)); - /* - * 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; 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 @@ if_clone_detach(struct if_clone *ifc) 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 @@ ifc_name2unit(const char *name, int *uni int ifc_alloc_unit(struct if_clone *ifc, int *unit) { - int wildcard, bytoff, bitoff; - int err = 0; - - IF_CLONE_LOCK(ifc); + char name[IFNAMSIZ]; + int wildcard; - 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; - } - - if (*unit > ifc->ifc_maxunit) { - err = ENOSPC; - goto done; + *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 (!wildcard) { - bytoff = *unit >> 3; - bitoff = *unit - (bytoff << 3); + 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((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); + IF_CLONE_ADDREF(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 Modified: head/sys/net/if_clone.h ============================================================================== --- head/sys/net/if_clone.h Mon Nov 28 14:39:56 2011 (r228070) +++ head/sys/net/if_clone.h Mon Nov 28 14:44:59 2011 (r228071) @@ -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 @@ struct if_clone { 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 @@ struct if_clone { 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); _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org" >Unformatted: