From owner-freebsd-current@FreeBSD.ORG Tue Nov 29 00:07:17 2011 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1FEE1106566B; Tue, 29 Nov 2011 00:07:17 +0000 (UTC) (envelope-from Daan@vitsch.nl) Received: from VM01.VEHosting.nl (VM016.VEHosting.nl [IPv6:2001:1af8:2100:b020::140]) by mx1.freebsd.org (Postfix) with ESMTP id 7DC7D8FC08; Tue, 29 Nov 2011 00:07:16 +0000 (UTC) Received: from [2001:470:d233:2:224:8cff:fe82:66cd] ([IPv6:2001:470:d233:2:224:8cff:fe82:66cd]) (authenticated bits=0) by VM01.VEHosting.nl (8.14.3/8.13.8) with ESMTP id pAT07I3x005223; Tue, 29 Nov 2011 01:07:18 +0100 (CET) (envelope-from Daan@vitsch.nl) From: Daan Vreeken Organization: Vitsch Electronics To: Gleb Smirnoff Date: Tue, 29 Nov 2011 01:07:13 +0100 User-Agent: KMail/1.9.10 References: <201111240928.51162.Daan@vitsch.nl> <20111125131935.GP96616@FreeBSD.org> <20111125143257.GR96616@FreeBSD.org> In-Reply-To: <20111125143257.GR96616@FreeBSD.org> MIME-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_xIC1OndPncenpNF" Message-Id: <201111290107.13631.Daan@vitsch.nl> x-ve-auth-version: mi-1.1.5 2011-02-07 - Copyright (c) 2008, 2011 - Daan Vreeken - VEHosting x-ve-auth: authenticated as 'pa4dan' on VM01.VEHosting.nl Cc: FreeBSD Current Subject: Re: if_clone.c allows creating multiple interfaces with the same name X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Nov 2011 00:07:17 -0000 --Boundary-00=_xIC1OndPncenpNF Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Content-Disposition: inline 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 --Boundary-00=_xIC1OndPncenpNF Content-Type: text/x-diff; charset="iso-8859-1"; name="if_clone-ENOSPC-and-unr-leak-fix-2011-11-29.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="if_clone-ENOSPC-and-unr-leak-fix-2011-11-29.diff" 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 == -1) - return (EEXIST); + ret = alloc_unr_specific(ifc->ifc_unrhdr, unit); + if (ret == -1) { + if (wildcard) { + /* unit number already claimed. try next */ + unit++; + goto retry; + } else { + /* specified unit number already claimed */ + return (EEXIST); + } + } } - snprintf(name, IFNAMSIZ, "%s%d", ifc->ifc_name, *unit); + /* if we reach this point, a unit number has been allocated */ + 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); + /* name is already taken */ + free_unr(ifc->ifc_unrhdr, unit); + if (wildcard) { + /* increment unit number and try again */ + unit++; + goto retry; + } + return (EEXIST); } + *unit_nr = unit; IF_CLONE_ADDREF(ifc); --Boundary-00=_xIC1OndPncenpNF--