Skip site navigation (1)Skip section navigation (2) 
Date:      Tue, 29 Nov 2011 01:07:13 +0100
From:      Daan Vreeken <Daan@vitsch.nl>
To:        Gleb Smirnoff <glebius@freebsd.org>
Cc:        FreeBSD Current <freebsd-current@freebsd.org>
Subject:   Re: if_clone.c allows creating multiple interfaces with the same name
Message-ID:  <201111290107.13631.Daan@vitsch.nl>
In-Reply-To: <20111125143257.GR96616@FreeBSD.org>
References:  <201111240928.51162.Daan@vitsch.nl> <20111125131935.GP96616@FreeBSD.org> <20111125143257.GR96616@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help 
--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--

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201111290107.13631.Daan>