[patch 1/1][CORE] resend - fix free_netdev on register_netdev failure

[patch 1/1][CORE] resend - fix free_netdev on register_netdev failure

[Daniel Lezcano]

[-- Attachment #1: fix-registering-failure.patch --]
[-- Type: text/plain, Size: 4866 bytes --]

Point 1:
The unregistering of a network device schedule a netdev_run_todo.
This function calls dev->destructor when it is set and the
destructor calls free_netdev.

Point 2:
In the case of an initialization of a network device the usual code
is:
 * alloc_netdev
 * register_netdev
    -> if this one fails, call free_netdev and exit with error.

Point 3:
In the register_netdevice function at the later state, when the device
is at the registered state, a call to the netdevice_notifiers is made.
If one of the notification falls into an error, a rollback to the
registered state is done using unregister_netdevice.

Conclusion:
When a network device fails to register during initialization because
one network subsystem returned an error during a notification call
chain, the network device is freed twice because of fact 1 and fact 2.
The second free_netdev will be done with an invalid pointer.

Proposed solution:
The following patch move all the code of unregister_netdevice *except* 
the call to net_set_todo, to a new function "rollback_registered".

The following functions are changed in this way:
 * register_netdevice: calls rollback_registered when a notification fails
 * unregister_netdevice: calls rollback_register + net_set_todo, the call
                         order to net_set_todo is changed because it is the
                         latest now. Since it justs add an element to a list
                         that should not break anything.

Signed-off-by: Daniel Lezcano <dlezcano@fr.ibm.com>
---
 net/core/dev.c |  112 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 59 insertions(+), 53 deletions(-)

Index: net-2.6/net/core/dev.c
===================================================================
--- net-2.6.orig/net/core/dev.c
+++ net-2.6/net/core/dev.c
@@ -3496,6 +3496,60 @@ static void net_set_todo(struct net_devi
 	spin_unlock(&net_todo_list_lock);
 }
 
+static void rollback_registered(struct net_device *dev)
+{
+	BUG_ON(dev_boot_phase);
+	ASSERT_RTNL();
+
+	/* Some devices call without registering for initialization unwind. */
+	if (dev->reg_state == NETREG_UNINITIALIZED) {
+		printk(KERN_DEBUG "unregister_netdevice: device %s/%p never "
+				  "was registered\n", dev->name, dev);
+
+		WARN_ON(1);
+		return;
+	}
+
+	BUG_ON(dev->reg_state != NETREG_REGISTERED);
+
+	/* If device is running, close it first. */
+	dev_close(dev);
+
+	/* And unlink it from device chain. */
+	unlist_netdevice(dev);
+
+	dev->reg_state = NETREG_UNREGISTERING;
+
+	synchronize_net();
+
+	/* Shutdown queueing discipline. */
+	dev_shutdown(dev);
+
+
+	/* Notify protocols, that we are about to destroy
+	   this device. They should clean all the things.
+	*/
+	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
+
+	/*
+	 *	Flush the unicast and multicast chains
+	 */
+	dev_addr_discard(dev);
+
+	if (dev->uninit)
+		dev->uninit(dev);
+
+	/* Notifier chain MUST detach us from master device. */
+	BUG_TRAP(!dev->master);
+
+	/* Remove entries from kobject tree */
+	netdev_unregister_kobject(dev);
+
+	synchronize_net();
+
+	dev_put(dev);
+}
+
 /**
  *	register_netdevice	- register a network device
  *	@dev: device to register
@@ -3633,8 +3687,10 @@ int register_netdevice(struct net_device
 	/* Notify protocols, that a new device appeared. */
 	ret = call_netdevice_notifiers(NETDEV_REGISTER, dev);
 	ret = notifier_to_errno(ret);
-	if (ret)
-		unregister_netdevice(dev);
+	if (ret) {
+		rollback_registered(dev);
+		dev->reg_state = NETREG_UNREGISTERED;
+	}
 
 out:
 	return ret;
@@ -3911,59 +3967,9 @@ void synchronize_net(void)
 
 void unregister_netdevice(struct net_device *dev)
 {
-	BUG_ON(dev_boot_phase);
-	ASSERT_RTNL();
-
-	/* Some devices call without registering for initialization unwind. */
-	if (dev->reg_state == NETREG_UNINITIALIZED) {
-		printk(KERN_DEBUG "unregister_netdevice: device %s/%p never "
-				  "was registered\n", dev->name, dev);
-
-		WARN_ON(1);
-		return;
-	}
-
-	BUG_ON(dev->reg_state != NETREG_REGISTERED);
-
-	/* If device is running, close it first. */
-	dev_close(dev);
-
-	/* And unlink it from device chain. */
-	unlist_netdevice(dev);
-
-	dev->reg_state = NETREG_UNREGISTERING;
-
-	synchronize_net();
-
-	/* Shutdown queueing discipline. */
-	dev_shutdown(dev);
-
-
-	/* Notify protocols, that we are about to destroy
-	   this device. They should clean all the things.
-	*/
-	call_netdevice_notifiers(NETDEV_UNREGISTER, dev);
-
-	/*
-	 *	Flush the unicast and multicast chains
-	 */
-	dev_addr_discard(dev);
-
-	if (dev->uninit)
-		dev->uninit(dev);
-
-	/* Notifier chain MUST detach us from master device. */
-	BUG_TRAP(!dev->master);
-
-	/* Remove entries from kobject tree */
-	netdev_unregister_kobject(dev);
-
+	rollback_registered(dev);
 	/* Finish processing unregister after unlock */
 	net_set_todo(dev);
-
-	synchronize_net();
-
-	dev_put(dev);
 }
 
 /**

-- 

Re: [patch 1/1][CORE] resend - fix free_netdev on register_netdev failure

[David Miller]

From: Daniel Lezcano <dlezcano@fr.ibm.com>
Date: Tue, 30 Oct 2007 17:19:26 +0100

> Point 1:
> The unregistering of a network device schedule a netdev_run_todo.
> This function calls dev->destructor when it is set and the
> destructor calls free_netdev.
> 
> Point 2:
> In the case of an initialization of a network device the usual code
> is:
>  * alloc_netdev
>  * register_netdev
>     -> if this one fails, call free_netdev and exit with error.
> 
> Point 3:
> In the register_netdevice function at the later state, when the device
> is at the registered state, a call to the netdevice_notifiers is made.
> If one of the notification falls into an error, a rollback to the
> registered state is done using unregister_netdevice.
> 
> Conclusion:
> When a network device fails to register during initialization because
> one network subsystem returned an error during a notification call
> chain, the network device is freed twice because of fact 1 and fact 2.
> The second free_netdev will be done with an invalid pointer.
> 
> Proposed solution:
> The following patch move all the code of unregister_netdevice *except* 
> the call to net_set_todo, to a new function "rollback_registered".
> 
> The following functions are changed in this way:
>  * register_netdevice: calls rollback_registered when a notification fails
>  * unregister_netdevice: calls rollback_register + net_set_todo, the call
>                          order to net_set_todo is changed because it is the
>                          latest now. Since it justs add an element to a list
>                          that should not break anything.
> 
> Signed-off-by: Daniel Lezcano <dlezcano@fr.ibm.com>

Looks good, applied.

Re: [patch 1/1][CORE] resend - fix free_netdev on register_netdev failure

[Stephen Hemminger]

On Tue, 30 Oct 2007 15:38:30 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Daniel Lezcano <dlezcano@fr.ibm.com>
> Date: Tue, 30 Oct 2007 17:19:26 +0100
> 
> > Point 1:
> > The unregistering of a network device schedule a netdev_run_todo.
> > This function calls dev->destructor when it is set and the
> > destructor calls free_netdev.
> > 
> > Point 2:
> > In the case of an initialization of a network device the usual code
> > is:
> >  * alloc_netdev
> >  * register_netdev
> >     -> if this one fails, call free_netdev and exit with error.
> > 
> > Point 3:
> > In the register_netdevice function at the later state, when the device
> > is at the registered state, a call to the netdevice_notifiers is made.
> > If one of the notification falls into an error, a rollback to the
> > registered state is done using unregister_netdevice.
> > 
> > Conclusion:
> > When a network device fails to register during initialization because
> > one network subsystem returned an error during a notification call
> > chain, the network device is freed twice because of fact 1 and fact 2.
> > The second free_netdev will be done with an invalid pointer.
> > 
> > Proposed solution:
> > The following patch move all the code of unregister_netdevice *except* 
> > the call to net_set_todo, to a new function "rollback_registered".
> > 
> > The following functions are changed in this way:
> >  * register_netdevice: calls rollback_registered when a notification fails
> >  * unregister_netdevice: calls rollback_register + net_set_todo, the call
> >                          order to net_set_todo is changed because it is the
> >                          latest now. Since it justs add an element to a list
> >                          that should not break anything.
> > 
> > Signed-off-by: Daniel Lezcano <dlezcano@fr.ibm.com>
> 
> Looks good, applied.

Agreed, but I wish the unwind code wouldn't have to be separate function.

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