Re: if_clone.c allows creating multiple interfaces with the same name

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