[PATCH] bridge: fix locking and memory leak in br_add_bridge

[PATCH] bridge: fix locking and memory leak in br_add_bridge

[Jiri Benc]

There are several problems in error handling in br_add_bridge:
- when dev_alloc_name fails, allocated net_device is not freed
- unregister_netdev is called when rtnl lock is held
- free_netdev is called before netdev_run_todo has a chance to be run after
  unregistering net_device

This patch should fix these issues.

Signed-off-by: Jiri Benc <jbenc@suse.cz>

---

 net/bridge/br_if.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

--- dscape.orig/net/bridge/br_if.c
+++ dscape/net/bridge/br_if.c
@@ -306,20 +306,19 @@ int br_add_bridge(const char *name)
 
 	ret = register_netdevice(dev);
 	if (ret)
-		goto err2;
+		goto err1;
 
 	ret = br_sysfs_addbr(dev);
 	if (ret)
-		goto err3;
+		goto err2;
 	rtnl_unlock();
 	return 0;
 
- err3:
-	unregister_netdev(dev);
  err2:
-	free_netdev(dev);
+	unregister_netdevice(dev);
  err1:
 	rtnl_unlock();
+	free_netdev(dev);
 	return ret;
 }
 


-- 
Jiri Benc
SUSE Labs

Re: [PATCH] bridge: fix locking and memory leak in br_add_bridge

[Jiri Benc]

On Thu, 25 May 2006 15:31:24 +0200, Jiri Benc wrote:
>   err2:
> -	free_netdev(dev);
> +	unregister_netdevice(dev);
>   err1:
>  	rtnl_unlock();
> +	free_netdev(dev);
>  	return ret;

Sorry, this is wrong. I didn't notice that br_dev_setup sets
dev->destructor to free_netdev. Corrected patch will follow.

 Jiri

-- 
Jiri Benc
SUSE Labs

[PATCH v2] bridge: fix locking and memory leak in br_add_bridge

[Jiri Benc]

There are several bugs in error handling in br_add_bridge:
- when dev_alloc_name fails, allocated net_device is not freed
- unregister_netdev is called when rtnl lock is held
- free_netdev is called before netdev_run_todo has a chance to be run after
  unregistering net_device

This patch should fix these issues.

Signed-off-by: Jiri Benc <jbenc@suse.cz>

---

 net/bridge/br_if.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

--- dscape.orig/net/bridge/br_if.c
+++ dscape/net/bridge/br_if.c
@@ -301,25 +301,22 @@ int br_add_bridge(const char *name)
 	if (strchr(dev->name, '%')) {
 		ret = dev_alloc_name(dev, dev->name);
 		if (ret < 0)
-			goto err1;
+			goto err;
 	}
 
 	ret = register_netdevice(dev);
 	if (ret)
-		goto err2;
+		goto err;
 
 	ret = br_sysfs_addbr(dev);
 	if (ret)
-		goto err3;
+		unregister_netdevice(dev);
 	rtnl_unlock();
-	return 0;
+	return ret;
 
- err3:
-	unregister_netdev(dev);
- err2:
-	free_netdev(dev);
- err1:
+ err:
 	rtnl_unlock();
+	free_netdev(dev);
 	return ret;
 }
 

-- 
Jiri Benc
SUSE Labs

Re: [PATCH v2] bridge: fix locking and memory leak in br_add_bridge

[Stephen Hemminger]

There are several bugs in error handling in br_add_bridge:
- when dev_alloc_name fails, allocated net_device is not freed
- unregister_netdev is called when rtnl lock is held
- free_netdev is called before netdev_run_todo has a chance to be run after
  unregistering net_device

This patch should fix these issues.
Minor rearrangement of to use common path of earlier patch. Dave please apply.

Signed-off-by: Jiri Benc <jbenc@suse.cz>
Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

--- br.orig/net/bridge/br_if.c
+++ br/net/bridge/br_if.c
@@ -301,25 +301,20 @@ int br_add_bridge(const char *name)
 	if (strchr(dev->name, '%')) {
 		ret = dev_alloc_name(dev, dev->name);
 		if (ret < 0)
-			goto err1;
+			goto err;
 	}
 
 	ret = register_netdevice(dev);
 	if (ret)
-		goto err2;
+		goto err;
 
 	ret = br_sysfs_addbr(dev);
 	if (ret)
-		goto err3;
-	rtnl_unlock();
-	return 0;
-
- err3:
-	unregister_netdev(dev);
- err2:
-	free_netdev(dev);
- err1:
+		unregister_netdevice(dev);
+ err:
 	rtnl_unlock();
+	if (ret)
+		free_netdev(dev);
 	return ret;
 }
 

Re: [PATCH v2] bridge: fix locking and memory leak in br_add_bridge

[Jiri Benc]

On Thu, 25 May 2006 09:23:31 -0700, Stephen Hemminger wrote:
> +		unregister_netdevice(dev);
> + err:
>  	rtnl_unlock();
> +	if (ret)
> +		free_netdev(dev);
>  	return ret;

I don't think this is correct. netdev_run_todo calls dev->destructor
which is set to free_netdev by br_dev_setup. So you're calling
free_netdev on already freed device.

 Jiri

-- 
Jiri Benc
SUSE Labs

Re: [PATCH v2] bridge: fix locking and memory leak in br_add_bridge

[David Miller]

From: Jiri Benc <jbenc@suse.cz>
Date: Fri, 26 May 2006 11:08:06 +0200

> On Thu, 25 May 2006 09:23:31 -0700, Stephen Hemminger wrote:
> > +		unregister_netdevice(dev);
> > + err:
> >  	rtnl_unlock();
> > +	if (ret)
> > +		free_netdev(dev);
> >  	return ret;
> 
> I don't think this is correct. netdev_run_todo calls dev->destructor
> which is set to free_netdev by br_dev_setup. So you're calling
> free_netdev on already freed device.

Right, we need a more correct version of this bug fix. :)

Re: [PATCH v2] bridge: fix locking and memory leak in br_add_bridge

[Jiri Benc]

Fri, 26 May 2006 19:26:11 -0700 (PDT), David Miller pise:
> From: Jiri Benc <jbenc@suse.cz>
> Date: Fri, 26 May 2006 11:08:06 +0200
> > I don't think this is correct. netdev_run_todo calls dev->destructor
> > which is set to free_netdev by br_dev_setup. So you're calling
> > free_netdev on already freed device.
> 
> Right, we need a more correct version of this bug fix. :)

I think the second version I sent
(http://www.spinics.net/lists/netdev/msg05111.html) should be correct.

 Jiri

-- 
Jiri Benc
SUSE Labs

[PATCH v3] bridge: fix locking and memory leak in br_add_bridge

[Stephen Hemminger]

There are several bugs in error handling in br_add_bridge:
- when dev_alloc_name fails, allocated net_device is not freed
- unregister_netdev is called when rtnl lock is held
- free_netdev is called before netdev_run_todo has a chance to be run after
  unregistering net_device

Signed-off-by: Jiri Benc <jbenc@suse.cz>
Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
--- br.orig/net/bridge/br_if.c
+++ br/net/bridge/br_if.c
@@ -300,25 +300,20 @@ int br_add_bridge(const char *name)
 	rtnl_lock();
 	if (strchr(dev->name, '%')) {
 		ret = dev_alloc_name(dev, dev->name);
-		if (ret < 0)
-			goto err1;
+		if (ret < 0) {
+			free_netdev(dev);
+			goto out;
+		}
 	}
 
 	ret = register_netdevice(dev);
 	if (ret)
-		goto err2;
+		goto out;
 
 	ret = br_sysfs_addbr(dev);
 	if (ret)
-		goto err3;
-	rtnl_unlock();
-	return 0;
-
- err3:
-	unregister_netdev(dev);
- err2:
-	free_netdev(dev);
- err1:
+		unregister_netdevice(dev);
+ out:
 	rtnl_unlock();
 	return ret;
 }

Re: [PATCH v3] bridge: fix locking and memory leak in br_add_bridge

[David Miller]

From: Stephen Hemminger <shemminger@osdl.org>
Date: Sat, 27 May 2006 09:45:28 -0700

> There are several bugs in error handling in br_add_bridge:
> - when dev_alloc_name fails, allocated net_device is not freed
> - unregister_netdev is called when rtnl lock is held
> - free_netdev is called before netdev_run_todo has a chance to be run after
>   unregistering net_device
> 
> Signed-off-by: Jiri Benc <jbenc@suse.cz>
> Signed-off-by: Stephen Hemminger <shemminger@osdl.org>

Applied, thanks a lot.