netdev - Re: [PATCH] bond_alb: don't disable softirq under bond_alb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <15858.1326137789@death>
Date:	Mon, 09 Jan 2012 11:36:29 -0800
From:	Jay Vosburgh <fubar@...ibm.com>
To:	Maxim Uvarov <maxim.uvarov@...cle.com>
cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	andy@...yhouse.net, amwang@...hat.com
Subject: Re: [PATCH] bond_alb: don't disable softirq under bond_alb_xmit

Maxim Uvarov <maxim.uvarov@...cle.com> wrote:

>On 01/07/2012 10:14 AM, David Miller wrote:
>> From: Jay Vosburgh<fubar@...ibm.com>
>> Date: Fri, 06 Jan 2012 13:33:25 -0800
>>
>>> Maxim Uvarov<maxim.uvarov@...cle.com>  wrote:
>>>
>>>> No need to lock soft irqs under bond_alb_xmit()
>>>> which already has softirq disabled.
>>>
>>> 	In commit:
>>>
>>> commit 6603a6f25e4bca922a7dfbf0bf03072d98850176
>>> Author: Jay Vosburgh<fubar@...ibm.com>
>>> Date:   Wed Oct 17 17:37:50 2007 -0700
>>>
>>>      bonding: Convert more locks to _bh, acquire rtnl, for new locking
>>>
>>>          Convert more lock acquisitions to _bh flavor to avoid deadlock
>>>      with workqueue activity and add acquisition of RTNL in appropriate places.
>>>      Affects ALB mode, as well as core bonding functions and sysfs.
>>>
>>>      Signed-off-by: Andy Gospodarek<andy@...yhouse.net>
>>>      Signed-off-by: Jay Vosburgh<fubar@...ibm.com>
>>>      Signed-off-by: Jeff Garzik<jeff@...zik.org>
>>>
>>> 	the _lock_tx_hashtbl was upgraded from regular to _bh to prevent
>>> deadlocks.  I don't recall right offhand what deadlock this prevented,
>>> but are we sure there are no possible issues with converting this lock
>>> back to a non-_bh acquisition?
>>
>> Maxim's patch is not changing the BH'ness of the list.
>>
>>
>> He's just avoiding a BH disable which is unnecessary because BH is
>> already disabled in the effected code path(s).
>>
>
>Yes, I only removed disabling BH for tlb_choose_channel(). In other places
>this lock still disables BH. This makes lock more accurate,
>because there are 2 paths for execution: 1. dev_queue_xmit() and BH
>are already disabled. 2. netpoll and irqs are disabled. So no need to
>enable/disable BH.

	The tlb_choose_channel and rlb_choose_channel parts look to be
as you describe, but you also modify tlb_clear_slave:

--- a/drivers/net/bonding/bond_alb.c
+++ b/drivers/net/bonding/bond_alb.c
@@ -135,7 +135,7 @@ static void tlb_clear_slave(struct bonding *bond, struct slave *slave, int save_
 	struct tlb_client_info *tx_hash_table;
 	u32 index;

-	_lock_tx_hashtbl(bond);
+	spin_lock(&(BOND_ALB_INFO(bond).tx_hashtbl_lock));

	This makes tlb_clear_slave acquire the tx_hashtbl_lock without
_bh.  The tlb_clear_slave function is called from multiple places
without already holding _bh, in addition to the call paths you list.
The cases I see are:

	bond_alb_monitor (which runs from a workqueue)

	bond_alb_handle_link_change (called from bond_miimon_commit,
from a workqueue)

	bond_alb_deinit_slave (called during slave removal)

	All three of these will call into tlb_clear_slave without
already holding something at _bh.  These paths do not enter
tlb_clear_slave through tlb_choose_channel or rlb_choose_channel.

	Are we sure this does not open a window wherein the non-_bh path
into tlb_clear_slave could deadlock against the with-_bh path?

	-J

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

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