Skip to contentSkip to conversation

dev_open() and dev_close() require RTNL

PATCH Applied Ben Hutchings 4 participants 18 messages May 6, 2008 – Jun 27, 2008

Subsystems (3)
Networking Network drivers X.25 stack
lore
Share & tools
lore

Ben Hutchings

author
#0 lore raw

cover dev_open() and dev_close() require RTNL

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.

Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
author
#1 lore raw

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

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 <[email protected]>
---
 net/core/dev.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
net/core/dev.c +4 -0
index d334446..e76f4fd 100644
@@ -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))
Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
maintainer Applied
#2 lore raw

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

From: Ben Hutchings <[email protected]>
Date: Tue, 6 May 2008 19:34:06 +0100

Quoted 4 lines

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 <[email protected]>

Applied, thanks Ben.

author
#3 lore raw

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

In reply to Ben Hutchings

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/qla3xxx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/qla3xxx.c +2 -0
index b7f7b22..bccee68 100644
@@ -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;
Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
Applied
#5 lore raw

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

In reply to Ben Hutchings
Quoted 12 lines · Ben Hutchings

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/qla3xxx.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/qla3xxx.c +0 -0
index b7f7b22..bccee68 100644

drivers/net/qla3xxx.c static int ql_cycle_adapter(struct ql3_adapter *qdev, int reset) · line 3707

Quoted 7 lines
drivers/net/qla3xxx.c +2 -0
@@ -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

author
#6 lore raw

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

In reply to Ben Hutchings

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/sky2.c +2 -0
index f226bca..9b046dd 100644
@@ -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();
 			}
 		}
 	}
Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
#7 lore raw

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

Quoted 13 lines · On Tue, 6 May 2008 19:39:58 +0100 Ben Hutchings <[email protected]>…

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/sky2.c +0 -0
index f226bca..9b046dd 100644

drivers/net/sky2.c static void sky2_restart(struct work_struct *work) · line 2960

Quoted 9 lines
drivers/net/sky2.c +2 -0
@@ -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.

#8 lore raw

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

In reply to Ben Hutchings
Quoted 13 lines · On Tue, 6 May 2008 19:39:58 +0100 Ben Hutchings <[email protected]>…

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/sky2.c +0 -0
index f226bca..9b046dd 100644

drivers/net/sky2.c static void sky2_restart(struct work_struct *work) · line 2960

Quoted 9 lines
drivers/net/sky2.c +2 -0
@@ -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();
}
author
#9 lore raw

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

Quoted 20 lines · Stephen Hemminger
Quoted 17 lines · On Tue, 6 May 2008 19:39:58 +0100 Ben Hutchings <[email protected]>…

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/sky2.c +1 -0
index f226bca..9b046dd 100644
@@ -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);
Quoted 8 lines

drivers/net/sky2.c static void sky2_restart(struct work_struct *work) · line 2960

Quoted 5 lines
drivers/net/sky2.c +1 -0
+				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.

Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
author
#10 lore raw

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

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/sky2.c +2 -0
index f226bca..b9bdf88 100644
@@ -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;
 			}
 		}
Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
#11 lore raw

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

Quoted 12 lines · Ben Hutchings

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/sky2.c +0 -0
index f226bca..b9bdf88 100644

drivers/net/sky2.c static int sky2_resume(struct pci_dev *pdev) · line 4402

Quoted 8 lines
drivers/net/sky2.c +2 -0
@@ -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...

#12 lore raw

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

Quoted 8 lines · On Tue, 10 Jun 2008 18:16:42 -0400 Jeff Garzik <[email protected]> wrote:
Quoted 7 lines · Ben Hutchings

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Quoted 17 lines

drivers/net/sky2.c static int sky2_resume(struct pci_dev *pdev) · line 4402

Quoted 13 lines
drivers/net/sky2.c +2 -0
index f226bca..b9bdf88 100644
@@ -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

Applied
#13 lore raw

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

In reply to Ben Hutchings
Quoted 12 lines · Ben Hutchings

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/sky2.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/net/sky2.c +0 -0
index f226bca..b9bdf88 100644

drivers/net/sky2.c static int sky2_resume(struct pci_dev *pdev) · line 4401

Quoted 7 lines
drivers/net/sky2.c +2 -0
@@ -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

author
#14 lore raw

PATCH 4/4 Hold RTNL while calling dev_close()

In reply to Ben Hutchings

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

Signed-off-by Ben Hutchings <[email protected]>
---
 drivers/net/wan/x25_asy.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/net/wan/x25_asy.c +3 -0
index 249e180..069f8bb 100644
@@ -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;
Signature · 2 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
author
#16 lore raw

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

Quoted 10 lines · David Miller

From: Ben Hutchings <[email protected]>
Date: Tue, 6 May 2008 19:41:48 +0100

Quoted 3 lines

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

Signed-off-by Ben Hutchings <[email protected]>
Acked-by David S. Miller <[email protected]>

Jeff, please grab this one too.

I don't see any sign that this has been applied. Could maybe do with an ACK or NAK from linux-x25?

Ben.

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