PATCH: bonding might sleep with lock held

PATCH: bonding might sleep with lock held

[Jay Vosburgh]

	In 802.3ad mode, the bond_close() function will eventually call
dev_remove_pack() with a lock held.  In turn, dev_remove_pack() calls
synchronize_net() which eventually might sleep in wait_for_completion().
Probably not good.

	This patch replaces dev_remove_pack() with __dev_remove_pack()
and adds a synchronize_net() call outside the lock.  The patch is
against 2.6.5, but applied to 2.6.6 for me.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com


diff -urN linux-2.6.5-virgin/drivers/net/bonding/bond_main.c linux-2.6.5/drivers/net/bonding/bond_main.c
--- linux-2.6.5-virgin/drivers/net/bonding/bond_main.c	2004-05-11 17:33:50.977235256 -0700
+++ linux-2.6.5/drivers/net/bonding/bond_main.c	2004-05-11 17:35:42.538275400 -0700
@@ -3495,7 +3495,7 @@
 /* unregister to receive lacpdus on a bond */
 static void bond_unregister_lacpdu(struct bonding *bond)
 {
-	dev_remove_pack(&(BOND_AD_INFO(bond).ad_pkt_type));
+	__dev_remove_pack(&(BOND_AD_INFO(bond).ad_pkt_type));
 }
 
 /*-------------------------- Device entry points ----------------------------*/
@@ -3580,6 +3580,12 @@
 
 	write_unlock_bh(&bond->lock);
 
+	/*
+	 * Sync outside the lock after __dev_remove_pack() in
+	 * bond_unregister_lacpdu()
+	 */
+	synchronize_net();
+
 	/* del_timer_sync must run without holding the bond->lock
 	 * because a running timer might be trying to hold it too
 	 */

Re: PATCH: bonding might sleep with lock held

[Andi Kleen]

On Wed, May 12, 2004 at 07:15:33PM -0700, Jay Vosburgh wrote:
> 
> 	In 802.3ad mode, the bond_close() function will eventually call
> dev_remove_pack() with a lock held.  In turn, dev_remove_pack() calls
> synchronize_net() which eventually might sleep in wait_for_completion().
> Probably not good.
> 
> 	This patch replaces dev_remove_pack() with __dev_remove_pack()
> and adds a synchronize_net() call outside the lock.  The patch is
> against 2.6.5, but applied to 2.6.6 for me.

I would rather fix bond_close to not call this with the lock hold.
You can just move the call a few lines up. dev->close has own
synchronization anyways and dev_remove_pack has a lock too, so this
should be safe.

Patch to do that appeneded.

-Andi


diff -u linux/drivers/net/bonding/bond_main.c-o linux/drivers/net/bonding/bond_main.c
--- linux/drivers/net/bonding/bond_main.c-o	1970-01-01 01:12:51.000000000 +0100
+++ linux/drivers/net/bonding/bond_main.c	2004-05-13 10:20:06.000000000 +0200
@@ -3566,15 +3566,15 @@
 {
 	struct bonding *bond = bond_dev->priv;
 
-	write_lock_bh(&bond->lock);
-
-	bond_mc_list_destroy(bond);
-
 	if (bond->params.mode == BOND_MODE_8023AD) {
 		/* Unregister the receive of LACPDUs */
 		bond_unregister_lacpdu(bond);
 	}
 
+	write_lock_bh(&bond->lock);
+
+	bond_mc_list_destroy(bond);
+
 	/* signal timers not to re-arm */
 	bond->kill_timers = 1;
 

Re: PATCH: bonding might sleep with lock held

[Jay Vosburgh]

Andi Kleen <ak@suse.de> wrote:
[...]
>> 	This patch replaces dev_remove_pack() with __dev_remove_pack()
>> and adds a synchronize_net() call outside the lock.  The patch is
>> against 2.6.5, but applied to 2.6.6 for me.
>
>I would rather fix bond_close to not call this with the lock hold.
>You can just move the call a few lines up. dev->close has own
>synchronization anyways and dev_remove_pack has a lock too, so this
>should be safe.

	It will race against the timers, although I don't see anything
in there offhand that would conflict.  It just feels somehow unclean to
do it without the lock.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: PATCH: bonding might sleep with lock held

[Andi Kleen]

On Thu, 13 May 2004 11:44:45 -0700
Jay Vosburgh <fubar@us.ibm.com> wrote:

> 	It will race against the timers, although I don't see anything
> in there offhand that would conflict.  It just feels somehow unclean to
> do it without the lock.

Why should the timers care if there can be a packet received from the 
network or not?  (this is all what dev_remove_pack prevents) 

-Andi

Re: PATCH: bonding might sleep with lock held

[Jay Vosburgh]

Andi Kleen <ak@suse.de> wrote:
>On Thu, 13 May 2004 11:44:45 -0700
>Jay Vosburgh <fubar@us.ibm.com> wrote:
>
>> 	It will race against the timers, although I don't see anything
>> in there offhand that would conflict.  It just feels somehow unclean to
>> do it without the lock.
>
>Why should the timers care if there can be a packet received from the 
>network or not?  (this is all what dev_remove_pack prevents) 

	The concern I had was that the timers might modify the fields of
the bonding structure accessed outside the lock, but they don't.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com