RTNL and flush_scheduled_work deadlocks

Ben found this but the problem seems pretty widespread.

The following places are subject to deadlock between flush_scheduled_work and the RTNL mutex. What can happen is that a work queue routine (like bridge port_carrier_check) is waiting forever for RTNL, and the driver routine has called flush_scheduled_work with RTNL held and is waiting for the work queue to clear.

Several other places have comments like: "can't call flush_scheduled_work here or it will deadlock". Most of the problem places are in device close routine. My recommendation would be to add a check for device netif_running in what ever work routine is used, and move the flush_scheduled_work to the remove routine.

8139too.c: rtl8139_close --> rtl8139_stop_thread
r8169.c: rtl8169_down
cassini.c: cas_change_mtu
iseries_veth.c: veth_stop_connection
s2io.c: s2io_close
sis190.c: sis190_down

I seem to be able to trigger this within about 1 minute on a particular 2.6.18.2 system with some 8139too devices, so if someone has a patch that could be tested, I'll gladly test it. For whatever reason, I haven't hit this problem on 2.6.20 yet, but that could easily be dumb luck, and I haven't been running .20 very much.

Ben Greear <[email protected]> :
[...]

Quoted 6 lines

I seem to be able to trigger this within about 1 minute on a particular 2.6.18.2 system with some 8139too devices, so if someone has a patch that could be tested, I'll gladly test it. For whatever reason, I haven't hit this problem on 2.6.20 yet, but that could easily be dumb luck, and I haven't been running .20 very much.

Bandaid below. It is not complete if your device hits the tx_watchdog hard but it should help.

flush_scheduled_work() in net_device->close has a slight tendency to deadlock with tasks on the workqueue that hold RTNL.

rtl8169_close/down simply need the recovery tasks to not meddle with the hardware while the device is going down.

Signed-off-by Francois Romieu <[email protected]>

---
 drivers/net/r8169.c |   25 ++++++++++++++++++-------
 1 files changed, 18 insertions(+), 7 deletions(-)

Your usual dont-flush_scheduled_work-with-RTNL-held stuff.

It is a bit different here since the thread runs permanently or is only occasionally kicked for recovery depending on the hardware revision.

Signed-off-by Francois Romieu <[email protected]>

---
 drivers/net/8139too.c |   40 +++++++++++++++++-----------------------
 1 files changed, 17 insertions(+), 23 deletions(-)

drivers/net/8139too.c +0 -0

index 35ad5cf..99304b2 100644

Quoted 6 lines

drivers/net/8139too.c +0 -0

index 35ad5cf..99304b2 100644
@@ -1109,6 +1109,8 @@ static void __devexit rtl8139_remove_one (struct pci_dev *pdev)
 

drivers/net/8139too.c static void __devexit rtl8139_remove_one (struct pci_dev *pdev) · line 1113

Quoted 4 lines

drivers/net/8139too.c +2 -0

 	assert (dev != NULL);
 
+	flush_scheduled_work();
+

IMHO there should be rather cancel_rearming_delayed_work instead of this.

Quoted 4 lines

drivers/net/8139too.c +0 -0

@@ -1603,18 +1605,21 @@ static void rtl8139_thread (struct work_struct *work)
 	struct net_device *dev = tp->mii.dev;
 	unsigned long thr_delay = next_tick;
 

drivers/net/8139too.c static void rtl8139_thread (struct work_struct *work) · line 1611

Quoted 4 lines

drivers/net/8139too.c +4 -0

+	rtnl_lock();
+
+	if (!netif_running(dev))
+		goto out_unlock;

I wonder, why you don't do netif_running before
rtnl_lock ? It's an atomic operation. And I'm not sure if increasing rtnl_lock range is really needed here.

Jarek Poplawski <[email protected]> :
[...]

Quoted 6 lines

Quoted 6 lines

drivers/net/8139too.c +0 -0

index 35ad5cf..99304b2 100644
@@ -1109,6 +1109,8 @@ static void __devexit rtl8139_remove_one (struct pci_dev *pdev)
 

Quoted 7 lines

drivers/net/8139too.c static void __devexit rtl8139_remove_one (struct pci_dev *pdev) · line 1113

Quoted 4 lines

drivers/net/8139too.c +2 -0

 	assert (dev != NULL);
 
+	flush_scheduled_work();
+

IMHO there should be rather cancel_rearming_delayed_work instead of this.

The delayed_work is initialized even if tp->have_thread is false, so cancel_rearming_delayed_work() will work, yes. Feel free to send a patch.

[...]

Quoted 4 lines

Quoted 4 lines

drivers/net/8139too.c +0 -0

@@ -1603,18 +1605,21 @@ static void rtl8139_thread (struct work_struct *work)
 	struct net_device *dev = tp->mii.dev;
 	unsigned long thr_delay = next_tick;
 

Quoted 8 lines

drivers/net/8139too.c static void rtl8139_thread (struct work_struct *work) · line 1611

Quoted 4 lines

drivers/net/8139too.c +4 -0

+	rtnl_lock();
+
+	if (!netif_running(dev))
+		goto out_unlock;

I wonder, why you don't do netif_running before
rtnl_lock ? It's an atomic operation. And I'm not sure if increasing rtnl_lock range is really needed here.

thread A: netif_running()
user task B: rtnl_lock()
user task B: dev->close()
user task B: rtnl_unlock()
thread A: rtnl_lock()
thread A: mess with closed device

Btw, the thread runs every 3*HZ at most.

You need to hold a dev reference (dev_hold) as well. to keep the device from disappearing. or do a flush_scheduled_work in the remove routine.

Jarek Poplawski <[email protected]> :

Quoted 4 lines

Quoted 4 lines

drivers/net/8139too.c +0 -0

@@ -1603,18 +1605,21 @@ static void rtl8139_thread (struct work_struct *work)
 	struct net_device *dev = tp->mii.dev;
 	unsigned long thr_delay = next_tick;
 

Quoted 8 lines

drivers/net/8139too.c static void rtl8139_thread (struct work_struct *work) · line 1611

Quoted 4 lines

drivers/net/8139too.c +4 -0

+	rtnl_lock();
+
+	if (!netif_running(dev))
+		goto out_unlock;

I wonder, why you don't do netif_running before
rtnl_lock ? It's an atomic operation. And I'm not sure if increasing rtnl_lock range is really needed here.

thread A: netif_running()
user task B: rtnl_lock()
user task B: dev->close()
user task B: rtnl_unlock()
thread A: rtnl_lock()
thread A: mess with closed device

Btw, the thread runs every 3*HZ at most.

Quoted 1 line

Btw, the thread runs every 3*HZ at most.

You are right (mostly)! But I think rtnl_lock is special and should be spared (even this 3*HZ) and here it's used for some mainly internal purpose (close synchronization). And it looks like mainly for this internal reason holding of rtnl_lock is increased. And because rtnl_lock is quite popular you have to take into consideration that after this 3*HZ it could spend some time waiting for the lock. So, maybe it would be nicer to check this netif_running twice (after rtnl_lock where needed), but maybe it's a mater of taste only, and yours is better, as well.

(Btw. I didn't verify this, but I hope you checked that places not under rtnl_lock before the patch are safe from some locking problems now.)

Your usual dont-flush_scheduled_work-with-RTNL-held stuff.

It is a bit different here since the thread runs permanently or is only occasionally kicked for recovery depending on the hardware revision.

It looks like this has not made it into the 2.6.20 stable series patches... Any reason not to add it there?

Ben found this but the problem seems pretty widespread.

The following places are subject to deadlock between flush_scheduled_work and the RTNL mutex. What can happen is that a work queue routine (like bridge port_carrier_check) is waiting forever for RTNL, and the driver routine has called flush_scheduled_work with RTNL held and is waiting for the work queue to clear.

Several other places have comments like: "can't call flush_scheduled_work here or it will deadlock". Most of the problem places are in device close routine. My recommendation would be to add a check for device netif_running in what ever work routine is used, and move the flush_scheduled_work to the remove routine.

8139too.c: rtl8139_close --> rtl8139_stop_thread
r8169.c: rtl8169_down
cassini.c: cas_change_mtu
iseries_veth.c: veth_stop_connection
s2io.c: s2io_close
sis190.c: sis190_down

Quoted 14 lines · Stephen Hemminger

Ben found this but the problem seems pretty widespread.

The following places are subject to deadlock between flush_scheduled_work and the RTNL mutex. What can happen is that a work queue routine (like bridge port_carrier_check) is waiting forever for RTNL, and the driver routine has called flush_scheduled_work with RTNL held and is waiting for the work queue to clear.

Several other places have comments like: "can't call flush_scheduled_work here or it will deadlock". Most of the problem places are in device close routine. My recommendation would be to add a check for device netif_running in what ever work routine is used, and move the flush_scheduled_work to the remove routine.

Quoted 8 lines

8139too.c: rtl8139_close --> rtl8139_stop_thread
r8169.c: rtl8169_down
cassini.c: cas_change_mtu
iseries_veth.c: veth_stop_connection
s2io.c: s2io_close
sis190.c: sis190_down

There is probably more than this...

Maybe there should be something like an ASSERT_NOT_RTNL() in the flush_scheduled_work()
method? If it's performance criticial, #ifdef it out if we're not debugging locks?

On Thu, Feb 15, 2007 at 11:40:32PM -0800, Ben Greear wrote:
...

Quoted 3 lines

Maybe there should be something like an ASSERT_NOT_RTNL() in the flush_scheduled_work() method? If it's performance criticial, #ifdef it out if we're not debugging locks?

Yes! I thought about the same (at first). But in my opinion it was not enough, so I thought about doing this in flush_workqueue. But in my next opinion it was not enough too. Now I think something like this should be done in rtnl_lock (under some debugging #if of course).

Cheers,
Jarek P.
-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Jarek Poplawski

On Thu, Feb 15, 2007 at 11:40:32PM -0800, Ben Greear wrote:
...

Quoted 5 lines

Maybe there should be something like an ASSERT_NOT_RTNL() in the flush_scheduled_work()
method? If it's performance criticial, #ifdef it out if we're not debugging locks?

Quoted 7 lines

Yes! I thought about the same (at first). But in my opinion it was not enough, so I thought about doing this in flush_workqueue. But in my next opinion it was not enough too. Now I think something like this should be done in rtnl_lock (under some debugging #if of course).

The reason these bugs have been hidden is that most of the time, there is nothing
on the pending work queue that will try to grab RTNL. But, the flush_work_queue
is still called with RTNL held, so an assert would find this much earlier than
waiting for someone to get lucky and actually catch (and debug and report) a deadlock...

I don't see how asserting it in the rtnl_lock would help anything, because at that
point we are about to deadlock anyway... (and this is probably very rare, as mentioned above.)

On Fri, Feb 16, 2007 at 12:23:05AM -0800, Ben Greear wrote:

Quoted 4 lines

I don't see how asserting it in the rtnl_lock would help anything, because at that
point we are about to deadlock anyway... (and this is probably very rare, as mentioned above.)

But it's happening now. Probably lockdep is not enough and rtnl_lock is probably used in too many places, so I meant some additional checks would be possible.

Quoted 2 lines · Jarek Poplawski

On Fri, Feb 16, 2007 at 12:23:05AM -0800, Ben Greear wrote:

...

Quoted 3 lines

Quoted 3 lines

I don't see how asserting it in the rtnl_lock would help anything, because at that point we are about to deadlock anyway... (and this is probably very rare, as mentioned above.)

Quoted 4 lines

But it's happening now. Probably lockdep is not enough and rtnl_lock is probably used in too many places, so I meant some additional checks would be possible.

And of course it already could be done by DEBUG_MUTEXES or DEBUG_SPINLOCK, so I gone too far and it's really bad idea.

Well, I had lockdep and all of the locking debugging I could find enabled, but
it did not catch this problem..I had to use sysctl -t and manually dig through the backtraces
to find the deadlock....

It may be that lockdep could be enhanced to catch this sort of thing....

Quoted 25 lines · Jarek Poplawski

Quoted 22 lines · Stephen Hemminger

Ben found this but the problem seems pretty widespread.

The following places are subject to deadlock between flush_scheduled_work and the RTNL mutex. What can happen is that a work queue routine (like bridge port_carrier_check) is waiting forever for RTNL, and the driver routine has called flush_scheduled_work with RTNL held and is waiting for the work queue to clear.

Several other places have comments like: "can't call flush_scheduled_work here or it will deadlock". Most of the problem places are in device close routine. My recommendation would be to add a check for device netif_running in what ever work routine is used, and move the flush_scheduled_work to the remove routine.

8139too.c: rtl8139_close --> rtl8139_stop_thread
r8169.c: rtl8169_down
cassini.c: cas_change_mtu
iseries_veth.c: veth_stop_connection
s2io.c: s2io_close
sis190.c: sis190_down

Quoted 2 lines

There is probably more than this...

Maybe there should be something like an ASSERT_NOT_RTNL() in the flush_scheduled_work()
method? If it's performance criticial, #ifdef it out if we're not debugging locks?

Quoted 3 lines · On Thu, 15 Feb 2007 23:40:32 -0800 Ben Greear <[email protected]> wrote:

Maybe there should be something like an ASSERT_NOT_RTNL() in the flush_scheduled_work() method? If it's performance criticial, #ifdef it out if we're not debugging locks?

You can't safely add a check like that. What if another cpu had acquired RTNL and was unrelated.

Quoted 9 lines · Stephen Hemminger

Quoted 5 lines · On Thu, 15 Feb 2007 23:40:32 -0800 Ben Greear <[email protected]> wrote:

Maybe there should be something like an ASSERT_NOT_RTNL() in the flush_scheduled_work()
method? If it's performance criticial, #ifdef it out if we're not debugging locks?

Quoted 2 lines

You can't safely add a check like that. What if another cpu had acquired RTNL and was unrelated.

I guess there isn't a way to see if *this* thread is the owner of the RTNL currently? I think lockdep knows the owner...maybe could query it somehow, or just save the owner in the mutex object when debugging is enabled...

Quoted 12 lines · Ben Greear

Quoted 10 lines · Stephen Hemminger

Quoted 4 lines · On Thu, 15 Feb 2007 23:40:32 -0800 Ben Greear <[email protected]> wrote:

Maybe there should be something like an ASSERT_NOT_RTNL() in the flush_scheduled_work() method? If it's performance criticial, #ifdef it out if we're not debugging locks?

You can't safely add a check like that. What if another cpu had acquired RTNL and was unrelated.

Quoted 4 lines

I guess there isn't a way to see if *this* thread is the owner of the RTNL currently? I think lockdep knows the owner...maybe could query it somehow, or just save the owner in the mutex object when debugging is enabled...

Here is my patch proposal to enable such thing
(and to make ASSERT_RTNL simpler btw.).

Signed-off-by Jarek Poplawski <[email protected]>

---

 include/linux/rtnetlink.h |   13 +++++++++++--
 net/core/rtnetlink.c      |   29 ++++++++++++++++++++++++++---
 2 files changed, 37 insertions(+), 5 deletions(-)

diff -Nurp linux-2.6.20-git13-/include/linux/rtnetlink.h linux-2.6.20-git13/include/linux/rtnetlink.h

include/linux/rtnetlink.h +11 -2

@@ -704,16 +704,25 @@ extern int rtnl_trylock(void);
 
 extern void rtnetlink_init(void);
 extern void __rtnl_unlock(void);
+extern int rtnl_assert(void);
 
 #define ASSERT_RTNL() do { \
-	if (unlikely(rtnl_trylock())) { \
-		rtnl_unlock(); \
+	if (unlikely(!rtnl_assert())) { \
 		printk(KERN_ERR "RTNL: assertion failed at %s (%d)\n", \
 		       __FILE__,  __LINE__); \
 		dump_stack(); \
 	} \
 } while(0)
 
+/* Current process shouldn't hold RTNL lock: */
+#define ASSERT_NOT_RTNL() do { \
+	if (unlikely(rtnl_assert())) { \
+		printk(KERN_ERR "NOT_RTNL: assertion failed at %s (%d)\n", \
+		       __FILE__,  __LINE__); \
+		dump_stack(); \
+	} \
+} while(0)
+
 #define BUG_TRAP(x) do { \
 	if (unlikely(!(x))) { \
 		printk(KERN_ERR "KERNEL: assertion (%s) failed at %s (%d)\n", \

diff -Nurp linux-2.6.20-git13-/net/core/rtnetlink.c linux-2.6.20-git13/net/core/rtnetlink.c

net/core/rtnetlink.c +26 -4

@@ -57,20 +57,33 @@
 #endif	/* CONFIG_NET_WIRELESS_RTNETLINK */
 
 static DEFINE_MUTEX(rtnl_mutex);
+static struct thread_info *rtnl_owner;
 static struct sock *rtnl;
 
 void rtnl_lock(void)
 {
+#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
+	WARN_ON(rtnl_owner == current_thread_info());
+#endif
 	mutex_lock(&rtnl_mutex);
+	rtnl_owner = current_thread_info();
 }
 
 void __rtnl_unlock(void)
 {
+#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
+	WARN_ON(rtnl_owner != current_thread_info());
+#endif
+	rtnl_owner = NULL;
 	mutex_unlock(&rtnl_mutex);
 }
 
 void rtnl_unlock(void)
 {
+#if !defined(CONFIG_DEBUG_MUTEXES) && !defined(CONFIG_DEBUG_SPINLOCK)
+	WARN_ON(rtnl_owner != current_thread_info());
+#endif
+	rtnl_owner = NULL;
 	mutex_unlock(&rtnl_mutex);
 	if (rtnl && rtnl->sk_receive_queue.qlen)
 		rtnl->sk_data_ready(rtnl, 0);

@@ -79,7 +92,16 @@ void rtnl_unlock(void)
 
 int rtnl_trylock(void)
 {
-	return mutex_trylock(&rtnl_mutex);
+	int ret;
+	
+	if ((ret = mutex_trylock(&rtnl_mutex)))
+		rtnl_owner = current_thread_info();
+	return ret;
+}
+
+int rtnl_assert(void)
+{
+	return (rtnl_owner == current_thread_info());
 }
 
 int rtattr_parse(struct rtattr *tb[], int maxattr, struct rtattr *rta, int len)

@@ -805,9 +827,9 @@ static void rtnetlink_rcv(struct sock *s
 	unsigned int qlen = 0;
 
 	do {
-		mutex_lock(&rtnl_mutex);
+		rtnl_lock();
 		netlink_run_queue(sk, &qlen, &rtnetlink_rcv_msg);
-		mutex_unlock(&rtnl_mutex);
+		__rtnl_unlock();
 
 		netdev_run_todo();
 	} while (qlen);

@@ -893,3 +915,4 @@ EXPORT_SYMBOL(rtnl_unlock);
 EXPORT_SYMBOL(rtnl_unicast);
 EXPORT_SYMBOL(rtnl_notify);
 EXPORT_SYMBOL(rtnl_set_sk_err);
+EXPORT_SYMBOL(rtnl_assert);
-

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 22 lines · Jarek Poplawski

Quoted 19 lines · Ben Greear

Quoted 12 lines · Stephen Hemminger

Quoted 6 lines · On Thu, 15 Feb 2007 23:40:32 -0800 Ben Greear <[email protected]> wrote:

Maybe there should be something like an ASSERT_NOT_RTNL() in the flush_scheduled_work()
method? If it's performance criticial, #ifdef it out if we're not debugging locks?

You can't safely add a check like that. What if another cpu had acquired RTNL and was unrelated.

I guess there isn't a way to see if *this* thread is the owner of the RTNL currently? I think lockdep knows the owner...maybe could query it somehow,
or just save the owner in the mutex object when debugging is enabled...

Quoted 3 lines

Here is my patch proposal to enable such thing
(and to make ASSERT_RTNL simpler btw.).

For performance reasons, I'd leave the rtnl_owner inside the #if debugging locking code....

You are also changing the semantics of ASSERT_RTNL (assert *this thread* has rtnl, from the
old behaviour: assert *some thread* has rtnl). It may be better this way, but it could break code that assumes the old behaviour.

On Sun, Feb 18, 2007 at 10:27:19PM -0800, Ben Greear wrote:

Quoted 4 lines

You are also changing the semantics of ASSERT_RTNL (assert *this thread* has rtnl, from the
old behaviour: assert *some thread* has rtnl). It may be better this way, but it could break code that assumes the old behaviour.

Quoted 1 line · Jarek Poplawski

On Fri, Feb 16, 2007 at 11:04:02AM -0800, Ben Greear wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 4 lines · On Thu, 15 Feb 2007 23:40:32 -0800 Ben Greear <[email protected]> wrote:

Maybe there should be something like an ASSERT_NOT_RTNL() in the flush_scheduled_work()
method? If it's performance criticial, #ifdef it out if we're not debugging locks?

For performance reasons, I'd leave the rtnl_owner inside the #if debugging locking code....

You are also changing the semantics of ASSERT_RTNL (assert *this thread* has rtnl, from the
old behaviour: assert *some thread* has rtnl). It may be better this way, but it could break code that assumes the old behaviour.

Ben

Quoted 9 lines · Stephen Hemminger

Quoted 5 lines · On Thu, 15 Feb 2007 23:40:32 -0800 Ben Greear <[email protected]> wrote:

Maybe there should be something like an ASSERT_NOT_RTNL() in the flush_scheduled_work()
method? If it's performance criticial, #ifdef it out if we're not debugging locks?

Quoted 2 lines

You can't safely add a check like that. What if another cpu had acquired RTNL and was unrelated.

I guess there isn't a way to see if *this* thread is the owner of the RTNL currently? I think lockdep knows the owner...maybe could query it somehow, or just save the owner in the mutex object when debugging is enabled...