[PATCH 0/4] dev_open() and dev_close() require RTNL

[PATCH 0/4] dev_open() and dev_close() require RTNL

[Ben Hutchings]

dev_open() and dev_close() clearly require the RTNL, but this isn't
commented or asserted.  The following series adds assertions and fixes
three drivers that call dev_close() without holding the RTNL.  The
driver changes are only compile-tested.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

[PATCH 1/4] Added ASSERT_RTNL() to dev_open() and dev_close().

[Ben Hutchings]

dev_open() and dev_close() must be called holding the RTNL, since they
call device functions and netdevice notifiers that are promised the RTNL.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 net/core/dev.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index d334446..e76f4fd 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -994,6 +994,8 @@ int dev_open(struct net_device *dev)
 {
 	int ret = 0;
 
+	ASSERT_RTNL();
+
 	/*
 	 *	Is it already up?
 	 */
@@ -1060,6 +1062,8 @@ int dev_open(struct net_device *dev)
  */
 int dev_close(struct net_device *dev)
 {
+	ASSERT_RTNL();
+
 	might_sleep();
 
 	if (!(dev->flags & IFF_UP))

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 1/4] Added ASSERT_RTNL() to dev_open() and dev_close().

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 6 May 2008 19:34:06 +0100

> dev_open() and dev_close() must be called holding the RTNL, since they
> call device functions and netdevice notifiers that are promised the RTNL.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Applied, thanks Ben.

[PATCH 2/4] qla3xxx: Hold RTNL while calling dev_close()

[Ben Hutchings]

dev_close() must be called holding the RTNL.  Compile-tested only.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/qla3xxx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/qla3xxx.c b/drivers/net/qla3xxx.c
index b7f7b22..bccee68 100644
--- a/drivers/net/qla3xxx.c
+++ b/drivers/net/qla3xxx.c
@@ -3701,7 +3701,9 @@ static int ql_cycle_adapter(struct ql3_adapter *qdev, int reset)
 		printk(KERN_ERR PFX
 				"%s: Driver up/down cycle failed, "
 				"closing device\n",qdev->ndev->name);
+		rtnl_lock();
 		dev_close(qdev->ndev);
+		rtnl_unlock();
 		return -1;
 	}
 	return 0;

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 2/4] qla3xxx: Hold RTNL while calling dev_close()

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 6 May 2008 19:36:26 +0100

> dev_close() must be called holding the RTNL.  Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Acked-by: David S. Miller <davem@davemloft.net>

Jeff, please pick this up if you haven't already.

Thanks!

Re: [PATCH 2/4] qla3xxx: Hold RTNL while calling dev_close()

[Jeff Garzik]

Ben Hutchings wrote:
> dev_close() must be called holding the RTNL.  Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/qla3xxx.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/qla3xxx.c b/drivers/net/qla3xxx.c
> index b7f7b22..bccee68 100644
> --- a/drivers/net/qla3xxx.c
> +++ b/drivers/net/qla3xxx.c
> @@ -3701,7 +3701,9 @@ static int ql_cycle_adapter(struct ql3_adapter *qdev, int reset)
>  		printk(KERN_ERR PFX
>  				"%s: Driver up/down cycle failed, "
>  				"closing device\n",qdev->ndev->name);
> +		rtnl_lock();
>  		dev_close(qdev->ndev);
> +		rtnl_unlock();
>  		return -1;

applied

[PATCH 3/4] sky2: Hold RTNL while calling dev_close()

[Ben Hutchings]

dev_close() must be called holding the RTNL.  Compile-tested only.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index f226bca..9b046dd 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -2952,7 +2952,9 @@ static void sky2_restart(struct work_struct *work)
 			if (err) {
 				printk(KERN_INFO PFX "%s: could not restart %d\n",
 				       dev->name, err);
+				rtnl_lock();
 				dev_close(dev);
+				rtnl_unlock();
 			}
 		}
 	}

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 3/4] sky2: Hold RTNL while calling dev_close()

[Stephen Hemminger]

On Tue, 6 May 2008 19:39:58 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> dev_close() must be called holding the RTNL.  Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/sky2.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
> index f226bca..9b046dd 100644
> --- a/drivers/net/sky2.c
> +++ b/drivers/net/sky2.c
> @@ -2952,7 +2952,9 @@ static void sky2_restart(struct work_struct *work)
>  			if (err) {
>  				printk(KERN_INFO PFX "%s: could not restart %d\n",
>  				       dev->name, err);
> +				rtnl_lock();
>  				dev_close(dev);
> +				rtnl_unlock();
>  			}
>  		}
>  	}
> 

Ack, but this is in a real corner case so no need to send it to stable.

Re: [PATCH 3/4] sky2: Hold RTNL while calling dev_close()

[Stephen Hemminger]

On Tue, 6 May 2008 19:39:58 +0100
Ben Hutchings <bhutchings@solarflare.com> wrote:

> dev_close() must be called holding the RTNL.  Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/sky2.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
> index f226bca..9b046dd 100644
> --- a/drivers/net/sky2.c
> +++ b/drivers/net/sky2.c
> @@ -2952,7 +2952,9 @@ static void sky2_restart(struct work_struct *work)
>  			if (err) {
>  				printk(KERN_INFO PFX "%s: could not restart %d\n",
>  				       dev->name, err);
> +				rtnl_lock();
>  				dev_close(dev);
> +				rtnl_unlock();
>  			}
>  		}
>  	}
> 

No this is bogus.  The rtnl mutex is already held.
Look a little wider

static void sky2_restart(struct work_struct *work)
{
	struct sky2_hw *hw = container_of(work, struct sky2_hw, restart_work);
	struct net_device *dev;
	int i, err;

	rtnl_lock();
	for (i = 0; i < hw->ports; i++) {
		dev = hw->dev[i];
		if (netif_running(dev))
			sky2_down(dev);
	}

	napi_disable(&hw->napi);
	sky2_write32(hw, B0_IMSK, 0);
	sky2_reset(hw);
	sky2_write32(hw, B0_IMSK, Y2_IS_BASE);
	napi_enable(&hw->napi);

	for (i = 0; i < hw->ports; i++) {
		dev = hw->dev[i];
		if (netif_running(dev)) {
			err = sky2_up(dev);
			if (err) {
				printk(KERN_INFO PFX "%s: could not restart %d\n",
				       dev->name, err);
				dev_close(dev);
			}
		}
	}

	rtnl_unlock();
}

Re: [PATCH 3/4] sky2: Hold RTNL while calling dev_close()

[Ben Hutchings]

Stephen Hemminger wrote:
> On Tue, 6 May 2008 19:39:58 +0100
> Ben Hutchings <bhutchings@solarflare.com> wrote:
> 
> > dev_close() must be called holding the RTNL.  Compile-tested only.
> > 
> > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> > ---
> >  drivers/net/sky2.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
> > index f226bca..9b046dd 100644
> > --- a/drivers/net/sky2.c
> > +++ b/drivers/net/sky2.c
> > @@ -2952,7 +2952,9 @@ static void sky2_restart(struct work_struct *work)
> >  			if (err) {
> >  				printk(KERN_INFO PFX "%s: could not restart %d\n",
> >  				       dev->name, err);
> > +				rtnl_lock();
> >  				dev_close(dev);
> > +				rtnl_unlock();
> >  			}
> >  		}
> >  	}
> > 
> 
> No this is bogus.  The rtnl mutex is already held.
> Look a little wider
<snip>

Sorry about that.  I made a list of functions to check and then edited
them in a second pass.  The function that needs locking in sky2.c is
sky2_resume() not sky2_restart().

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

[PATCH] sky2: Hold RTNL while calling dev_close() [2nd try]

[Ben Hutchings]

dev_close() must be called holding the RTNL.  Compile-tested only.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
index f226bca..b9bdf88 100644
--- a/drivers/net/sky2.c
+++ b/drivers/net/sky2.c
@@ -4395,7 +4395,9 @@ static int sky2_resume(struct pci_dev *pdev)
 			if (err) {
 				printk(KERN_ERR PFX "%s: could not up: %d\n",
 				       dev->name, err);
+				rtnl_lock();
 				dev_close(dev);
+				rtnl_unlock();
 				goto out;
 			}
 		}

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH] sky2: Hold RTNL while calling dev_close() [2nd try]

[Jeff Garzik]

Ben Hutchings wrote:
> dev_close() must be called holding the RTNL.  Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/sky2.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
> index f226bca..b9bdf88 100644
> --- a/drivers/net/sky2.c
> +++ b/drivers/net/sky2.c
> @@ -4395,7 +4395,9 @@ static int sky2_resume(struct pci_dev *pdev)
>  			if (err) {
>  				printk(KERN_ERR PFX "%s: could not up: %d\n",
>  				       dev->name, err);
> +				rtnl_lock();
>  				dev_close(dev);
> +				rtnl_unlock();
>  				goto out;
>  			}

stephen acked this?  I don't see that in my notes...

Re: [PATCH] sky2: Hold RTNL while calling dev_close() [2nd try]

[Stephen Hemminger]

On Tue, 10 Jun 2008 18:16:42 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Ben Hutchings wrote:
> > dev_close() must be called holding the RTNL.  Compile-tested only.
> > 
> > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> > ---
> >  drivers/net/sky2.c |    2 ++
> >  1 files changed, 2 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
> > index f226bca..b9bdf88 100644
> > --- a/drivers/net/sky2.c
> > +++ b/drivers/net/sky2.c
> > @@ -4395,7 +4395,9 @@ static int sky2_resume(struct pci_dev *pdev)
> >  			if (err) {
> >  				printk(KERN_ERR PFX "%s: could not up: %d\n",
> >  				       dev->name, err);
> > +				rtnl_lock();
> >  				dev_close(dev);
> > +				rtnl_unlock();
> >  				goto out;
> >  			}
> 
> stephen acked this?  I don't see that in my notes...
> 
> 

yes. this should go in

Re: [PATCH] sky2: Hold RTNL while calling dev_close() [2nd try]

[Jeff Garzik]

Ben Hutchings wrote:
> dev_close() must be called holding the RTNL.  Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/sky2.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/sky2.c b/drivers/net/sky2.c
> index f226bca..b9bdf88 100644
> --- a/drivers/net/sky2.c
> +++ b/drivers/net/sky2.c
> @@ -4395,7 +4395,9 @@ static int sky2_resume(struct pci_dev *pdev)
>  			if (err) {
>  				printk(KERN_ERR PFX "%s: could not up: %d\n",
>  				       dev->name, err);
> +				rtnl_lock();
>  				dev_close(dev);
> +				rtnl_unlock();
>  				goto out;

applied

[PATCH 4/4] Hold RTNL while calling dev_close()

[Ben Hutchings]

dev_close() must be called holding the RTNL.  Compile-tested only.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/wan/x25_asy.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/net/wan/x25_asy.c b/drivers/net/wan/x25_asy.c
index 249e180..069f8bb 100644
--- a/drivers/net/wan/x25_asy.c
+++ b/drivers/net/wan/x25_asy.c
@@ -32,6 +32,7 @@
 #include <linux/x25.h>
 #include <linux/lapb.h>
 #include <linux/init.h>
+#include <linux/rtnetlink.h>
 #include "x25_asy.h"
 
 #include <net/x25device.h>
@@ -601,8 +602,10 @@ static void x25_asy_close_tty(struct tty_struct *tty)
 	if (!sl || sl->magic != X25_ASY_MAGIC)
 		return;
 
+	rtnl_lock();
 	if (sl->dev->flags & IFF_UP)
 		dev_close(sl->dev);
+	rtnl_unlock();
 
 	tty->disc_data = NULL;
 	sl->tty = NULL;

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.

Re: [PATCH 4/4] Hold RTNL while calling dev_close()

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 6 May 2008 19:41:48 +0100

> dev_close() must be called holding the RTNL.  Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Acked-by: David S. Miller <davem@davemloft.net>

Jeff, please grab this one too.

Thanks.

Re: [PATCH 4/4] Hold RTNL while calling dev_close()

[Jeff Garzik]

Ben Hutchings wrote:
> dev_close() must be called holding the RTNL.  Compile-tested only.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
>  drivers/net/wan/x25_asy.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

applied