RE: [patch 29/32] NET: Fix race condition about network device name allocation.

RE: [patch 29/32] NET: Fix race condition about network device name allocation.

[koan]

Hello,

I get a compilation failure from the patch:

  CC      net/core/stream.o
  CC      net/core/scm.o
  CC      net/core/gen_stats.o
  CC      net/core/gen_estimator.o
  CC      net/core/sysctl_net_core.o
  CC      net/core/dev.o
net/core/dev.c: In function 'netdev_run_todo':
net/core/dev.c:3155: error: 'struct net_device' has no member named 'dev'
make[2]: *** [net/core/dev.o] Error 1
make[1]: *** [net/core] Error 2
make: *** [net] Error 2


Thanks, Jesse

(I am not on list, please CC me with replies)

Re: [patch 29/32] NET: Fix race condition about network device name allocation.

[Chris Wright]

* koan (koan00@gmail.com) wrote:
> I get a compilation failure from the patch:
> 
>  CC      net/core/stream.o
>  CC      net/core/scm.o
>  CC      net/core/gen_stats.o
>  CC      net/core/gen_estimator.o
>  CC      net/core/sysctl_net_core.o
>  CC      net/core/dev.o
> net/core/dev.c: In function 'netdev_run_todo':
> net/core/dev.c:3155: error: 'struct net_device' has no member named 'dev'
> make[2]: *** [net/core/dev.o] Error 1
> make[1]: *** [net/core] Error 2
> make: *** [net] Error 2

Yes, that's a terrible backport from me.  I'm testing this one now,
and may simply drop it.
--

From: Stephen Hemminger <shemminger@linux-foundation.org>

Kenji Kaneshige found this race between device removal and
registration.  On unregister it is possible for the old device to
exist, because sysfs file is still open.  A new device with 'eth%d'
will select the same name, but sysfs kobject register will fial.

The following changes the shutdown order slightly. It hold a removes
the sysfs entries earlier (on unregister_netdevice), but holds a
kobject reference.  Then when todo runs the actual last put free
happens.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
[chrisw: backport to 2.6.20]
Signed-off-by: Chris Wright <chrisw@sous-sol.org>

---
 net/core/dev.c       |   10 ++++++----
 net/core/net-sysfs.c |    8 +++++++-
 2 files changed, 13 insertions(+), 5 deletions(-)

--- linux-2.6.20.13.orig/net/core/dev.c
+++ linux-2.6.20.13/net/core/dev.c
@@ -3138,7 +3138,6 @@ void netdev_run_todo(void)
 			continue;
 		}
 
-		netdev_unregister_sysfs(dev);
 		dev->reg_state = NETREG_UNREGISTERED;
 
 		netdev_wait_allrefs(dev);
@@ -3149,11 +3148,11 @@ void netdev_run_todo(void)
 		BUG_TRAP(!dev->ip6_ptr);
 		BUG_TRAP(!dev->dn_ptr);
 
-		/* It must be the very last action,
-		 * after this 'dev' may point to freed up memory.
-		 */
 		if (dev->destructor)
 			dev->destructor(dev);
+
+		/* Free network device */
+		kobject_put(&dev->class_dev.kobj);
 	}
 
 out:
@@ -3310,6 +3309,9 @@ int unregister_netdevice(struct net_devi
 	/* Notifier chain MUST detach us from master device. */
 	BUG_TRAP(!dev->master);
 
+	/* Remove entries from sysfs */
+	netdev_unregister_sysfs(dev);
+
 	/* Finish processing unregister after unlock */
 	net_set_todo(dev);
 
--- linux-2.6.20.13.orig/net/core/net-sysfs.c
+++ linux-2.6.20.13/net/core/net-sysfs.c
@@ -440,9 +440,15 @@ static struct class net_class = {
 #endif
 };
 
+/* Delete sysfs entries but hold kobject reference until after all
+ * netdev references are gone.
+ */
 void netdev_unregister_sysfs(struct net_device * net)
 {
-	class_device_del(&(net->class_dev));
+	struct class_device *dev = &(net->class_dev);
+
+	kobject_get(&dev->kobj);
+	class_device_del(dev);
 }
 
 /* Create sysfs entries for network device. */

[patch 29/32] NET: Fix race condition about network device name allocation.

[Chris Wright]

[-- Attachment #1: net-fix-race-condition-about-network-device-name-allocation.patch --]
[-- Type: text/plain, Size: 2329 bytes --]

-stable review patch.  If anyone has any objections, please let us know.
---------------------

From: Stephen Hemminger <shemminger@linux-foundation.org>

Kenji Kaneshige found this race between device removal and
registration.  On unregister it is possible for the old device to
exist, because sysfs file is still open.  A new device with 'eth%d'
will select the same name, but sysfs kobject register will fial.

The following changes the shutdown order slightly. It hold a removes
the sysfs entries earlier (on unregister_netdevice), but holds a
kobject reference.  Then when todo runs the actual last put free
happens.

Signed-off-by: Stephen Hemminger <shemminger@linux-foundation.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
[chrisw: backport to 2.6.20]
Signed-off-by: Chris Wright <chrisw@sous-sol.org>

---
 net/core/dev.c       |   10 ++++++----
 net/core/net-sysfs.c |    8 +++++++-
 2 files changed, 13 insertions(+), 5 deletions(-)

--- linux-2.6.20.13.orig/net/core/dev.c
+++ linux-2.6.20.13/net/core/dev.c
@@ -3138,7 +3138,6 @@ void netdev_run_todo(void)
 			continue;
 		}
 
-		netdev_unregister_sysfs(dev);
 		dev->reg_state = NETREG_UNREGISTERED;
 
 		netdev_wait_allrefs(dev);
@@ -3149,11 +3148,11 @@ void netdev_run_todo(void)
 		BUG_TRAP(!dev->ip6_ptr);
 		BUG_TRAP(!dev->dn_ptr);
 
-		/* It must be the very last action,
-		 * after this 'dev' may point to freed up memory.
-		 */
 		if (dev->destructor)
 			dev->destructor(dev);
+
+		/* Free network device */
+		kobject_put(&dev->dev.kobj);
 	}
 
 out:
@@ -3310,6 +3309,9 @@ int unregister_netdevice(struct net_devi
 	/* Notifier chain MUST detach us from master device. */
 	BUG_TRAP(!dev->master);
 
+	/* Remove entries from sysfs */
+	netdev_unregister_sysfs(dev);
+
 	/* Finish processing unregister after unlock */
 	net_set_todo(dev);
 
--- linux-2.6.20.13.orig/net/core/net-sysfs.c
+++ linux-2.6.20.13/net/core/net-sysfs.c
@@ -440,9 +440,15 @@ static struct class net_class = {
 #endif
 };
 
+/* Delete sysfs entries but hold kobject reference until after all
+ * netdev references are gone.
+ */
 void netdev_unregister_sysfs(struct net_device * net)
 {
-	class_device_del(&(net->class_dev));
+	struct device *dev = &(net->dev);
+
+	kobject_get(&dev->kobj);
+	device_del(dev);
 }
 
 /* Create sysfs entries for network device. */

--