Hotplug race on name change

Hotplug race on name change

[Stephen Hemminger]

This:
> Without that patch, there is a race when registering network interfaces 
> and renaming it with udev rules, because initially the "address" in 
> sysfs doesn't contain useful data. See 
> http://marc.theaimsgroup.com/?t=114460338900002&r=1&w=2
> 
> Breaking the recommended way of assigning persistent network interface 
> names is, IMHO, a bug serious enough to be fixed in -stable.
> 
> Signed-off-by: Alexander E. Patrakov <patrakov@ums.usu.ru>
> 
> ---
> 
> --- linux-2.6.16.5/net/core/dev.c
> +++ linux-2.6.16.5/net/core/dev.c
> @@ -2932,11 +2932,11 @@
>  
>  		switch(dev->reg_state) {
>  		case NETREG_REGISTERING:
> +			dev->reg_state = NETREG_REGISTERED;
>  			err = netdev_register_sysfs(dev);
>  			if (err)
>  				printk(KERN_ERR "%s: failed sysfs registration (%d)\n",
>  				       dev->name, err);
> -			dev->reg_state = NETREG_REGISTERED;
>  			break;
>  
>  		case NETREG_UNREGISTERING:

Introduces new races in netdev_register_sysfs if the name changes, because
netdev_register_sysfs runs without RTNL at this point. So if some application gets
in and changes the device name while netdev_register_sysfs is running, then
the class_dev->class_id would end up not matching the netdevice->name.

Not a big issue since, hotplug doesn't get run until the device is registered.
Ideally, it would be possible to create the groups in the class device before it
was registered. It won't work with existing class device interface.

I am working on a patch to extend class_device to allow the creation of groups
to be atomic (like the attributes are).

Re: Hotplug race on name change

[Herbert Xu]

Stephen Hemminger <shemminger@osdl.org> wrote:
> 
> Introduces new races in netdev_register_sysfs if the name changes, because
> netdev_register_sysfs runs without RTNL at this point. So if some application gets
> in and changes the device name while netdev_register_sysfs is running, then
> the class_dev->class_id would end up not matching the netdevice->name.

Indeed.  It also seems to be possible now for someone to do an
unregister_netdevice while netdev_register_sysfs is still going.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH] netdev: hotplug napi race cleanup

[Stephen Hemminger]

This follows after the earlier two patches.

Change the initialization of the class device portion of the net device
to be done earlier, so that any races before registration completes are
harmless.  Add a mutex to avoid changes to netdevice during the
class device registration. 

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>


--- linux-2.6.orig/net/core/dev.c	2006-04-24 10:31:15.000000000 -0700
+++ linux-2.6/net/core/dev.c	2006-04-24 10:31:16.000000000 -0700
@@ -203,10 +203,12 @@
 
 #ifdef CONFIG_SYSFS
 extern int netdev_sysfs_init(void);
-extern int netdev_register_sysfs(struct net_device *);
-extern void netdev_unregister_sysfs(struct net_device *);
+extern void netdev_init_classdev(struct net_device *);
+#define netdev_register_sysfs(dev)	class_device_add(&(dev->class_dev))
+#define	netdev_unregister_sysfs(dev)	class_device_del(&(dev->class_dev))
 #else
 #define netdev_sysfs_init()	 	(0)
+#define netdev_init_classdev(dev)	do { } while(0)
 #define netdev_register_sysfs(dev)	(0)
 #define	netdev_unregister_sysfs(dev)	do { } while(0)
 #endif
@@ -2870,6 +2872,8 @@
 
 	set_bit(__LINK_STATE_PRESENT, &dev->state);
 
+	netdev_init_classdev(dev);
+
 	dev->next = NULL;
 	dev_init_scheduler(dev);
 	write_lock_bh(&dev_base_lock);
@@ -3047,7 +3051,10 @@
 			 * this is a delayed call after register_netdevice
 			 * so no way to tell device driver what is wrong.
 			 */
+			rtnl_lock();
 			err = netdev_register_sysfs(dev);
+			__rtnl_unlock();
+
 			if (err) {
 				printk(KERN_ERR "%s: failed sysfs registration (%d)\n",
 				       dev->name, err);
--- sky2-2.6.17.orig/net/core/net-sysfs.c	2006-04-24 10:31:14.000000000 -0700
+++ sky2-2.6.17/net/core/net-sysfs.c	2006-04-24 10:31:16.000000000 -0700
@@ -443,13 +443,8 @@
 #endif
 };
 
-void netdev_unregister_sysfs(struct net_device * net)
-{
-	class_device_del(&(net->class_dev));
-}
-
-/* Create sysfs entries for network device. */
-int netdev_register_sysfs(struct net_device *net)
+/* Setup class device */
+void netdev_init_classdev(struct net_device *net)
 {
 	struct class_device *class_dev = &(net->class_dev);
 	struct attribute_group **groups = net->sysfs_groups;
@@ -470,8 +465,6 @@
 	    || (net->wireless_handlers && net->wireless_handlers->get_wireless_stats))
 		*groups++ = &wireless_group;
 #endif
-
-	return class_device_add(class_dev);
 }
 
 int netdev_sysfs_init(void)

Re: [PATCH] netdev: hotplug napi race cleanup

[Andrew Morton]

Stephen Hemminger <shemminger@osdl.org> wrote:
>
> This follows after the earlier two patches.
> 
> Change the initialization of the class device portion of the net device
> to be done earlier, so that any races before registration completes are
> harmless.  Add a mutex to avoid changes to netdevice during the
> class device registration. 
> 

I had to fix up a reject in here.

> @@ -3047,7 +3051,10 @@
>  			 * this is a delayed call after register_netdevice
>  			 * so no way to tell device driver what is wrong.
>  			 */
> +			rtnl_lock();
>  			err = netdev_register_sysfs(dev);
> +			__rtnl_unlock();
> +
>  			if (err) {
>  				printk(KERN_ERR "%s: failed sysfs registration (%d)\n",
>  				       dev->name, err);

bix:/usr/src/25> grep 'this is a delayed call after' net/core/*.c patches/*.patch
bix:/usr/src/25> 

I cannot find that comment anywhere.

Re: [PATCH] netdev: hotplug napi race cleanup

[David S. Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Mon, 24 Apr 2006 15:23:41 -0700

> This follows after the earlier two patches.
> 
> Change the initialization of the class device portion of the net device
> to be done earlier, so that any races before registration completes are
> harmless.  Add a mutex to avoid changes to netdevice during the
> class device registration. 
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

Once Greg KH puts in the necessary infrastructure patches, I'll put
this one in too.

Re: [PATCH] netdev: hotplug napi race cleanup

[David S. Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Mon, 24 Apr 2006 15:23:41 -0700

> This follows after the earlier two patches.
> 
> Change the initialization of the class device portion of the net device
> to be done earlier, so that any races before registration completes are
> harmless.  Add a mutex to avoid changes to netdevice during the
> class device registration. 
> 
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

I'm not going to apply this patch and instead request that we think
about why this problem exists in the first place.

This patch is even stronger evidence that doing the sysfs registry in
the todo list processing is wrong.  If you can legally do this while
holding the rtnl semaphore, you can just as equally do it inside of
register_netdevice() which is where it truly belongs.

Then you can handle errors properly, unwind the state, and return the
error to the caller instead of just losing the error and leaving the
device in a half-registered state.

Re: [PATCH] netdev: hotplug napi race cleanup

[Stephen Hemminger]

On Sat, 06 May 2006 18:09:47 -0700 (PDT)
"David S. Miller" <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@osdl.org>
> Date: Mon, 24 Apr 2006 15:23:41 -0700
> 
> > This follows after the earlier two patches.
> > 
> > Change the initialization of the class device portion of the net device
> > to be done earlier, so that any races before registration completes are
> > harmless.  Add a mutex to avoid changes to netdevice during the
> > class device registration. 
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
> 
> I'm not going to apply this patch and instead request that we think
> about why this problem exists in the first place.
> 
> This patch is even stronger evidence that doing the sysfs registry in
> the todo list processing is wrong.  If you can legally do this while
> holding the rtnl semaphore, you can just as equally do it inside of
> register_netdevice() which is where it truly belongs.
> 
> Then you can handle errors properly, unwind the state, and return the
> error to the caller instead of just losing the error and leaving the
> device in a half-registered state.

The issue is are there network devices that can't sleep during
register_netdevice?

Re: [PATCH] netdev: hotplug napi race cleanup

[David S. Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Mon, 8 May 2006 09:54:58 -0700

> The issue is are there network devices that can't sleep during
> register_netdevice?

Oh right, I forgot about that.

Re: [PATCH] netdev: hotplug napi race cleanup

[Stephen Hemminger]

On Mon, 08 May 2006 11:37:31 -0700 (PDT)
"David S. Miller" <davem@davemloft.net> wrote:

> From: Stephen Hemminger <shemminger@osdl.org>
> Date: Mon, 8 May 2006 09:54:58 -0700
> 
> > The issue is are there network devices that can't sleep during
> > register_netdevice?
> 
> Oh right, I forgot about that.

We could do something like this in register_netdevice()

	if (in_atomic() || irqs_disabled())
		net_set_todo(dev);
	else {
		dev->reg_state = NETREG_REGISTERED;
		ret = netdev_register_sysfs(dev);
		if (ret) {
			... 
	}

It seems a bit grotty, and might cause pain later.