netdev - RE: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

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]

Date:	Thu, 7 Jan 2016 15:27:02 +0000
From:	"Tantilov, Emil S" <emil.s.tantilov@...el.com>
To:	Jay Vosburgh <jay.vosburgh@...onical.com>,
	"zyjzyj2000@...il.com" <zyjzyj2000@...il.com>
CC:	"mkubecek@...e.cz" <mkubecek@...e.cz>,
	"vfalico@...il.com" <vfalico@...il.com>,
	"gospo@...ulusnetworks.com" <gospo@...ulusnetworks.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"Shteinbock, Boris (Wind River)" <boris.shteinbock@...driver.com>
Subject: RE: [PATCH 1/1] bonding: restrict up state in 802.3ad mode

>-----Original Message-----
>From: Jay Vosburgh [mailto:jay.vosburgh@...onical.com]
>Sent: Wednesday, January 06, 2016 10:34 PM
>To: zyjzyj2000@...il.com
>Cc: Tantilov, Emil S; mkubecek@...e.cz; vfalico@...il.com;
>gospo@...ulusnetworks.com; netdev@...r.kernel.org; Shteinbock, Boris (Wind
>River)
>Subject: Re: [PATCH 1/1] bonding: restrict up state in 802.3ad mode
>
><zyjzyj2000@...il.com> wrote:
>
>>From: Zhu Yanjun <yanjun.zhu@...driver.com>
>>
>>In 802.3ad mode, the speed and duplex is needed. But in some NIC,
>>there is a time span between NIC up state and getting speed and duplex.
>>As such, sometimes a slave in 802.3ad mode is in up state without
>>speed and duplex. This will make bonding in 802.3ad mode can not
>>work well.
>
>	From my reading of Emil's comments in the discussion, I'm not
>sure the above is an accurate description of the problem.  If I'm
>understanding correctly, the cause is due to link flaps racing with the
>bonding monitor workqueue polling the state.  Is this correct?

That is correct.

>>To make bonding driver be compatible with more NICs, it is
>>necessary to restrict the up state in 802.3ad mode.
>>
>>Signed-off-by: Zhu Yanjun <yanjun.zhu@...driver.com>
>>---
>> drivers/net/bonding/bond_main.c |   11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>>diff --git a/drivers/net/bonding/bond_main.c
>b/drivers/net/bonding/bond_main.c
>>index 09f8a48..7df8af5 100644
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1991,6 +1991,17 @@ static int bond_miimon_inspect(struct bonding
>*bond)
>>
>> 		link_state = bond_check_dev_link(bond, slave->dev, 0);
>>
>>+		if ((BMSR_LSTATUS == link_state) &&
>>+		    (BOND_MODE(bond) == BOND_MODE_8023AD)) {
>>+			rtnl_lock();
>>+			bond_update_speed_duplex(slave);
>>+			rtnl_unlock();
>
>	This will add a round trip on the RTNL mutex for every miimon
>interval when the slave is carrier up.  At common miimon rates (10 - 50
>ms), this will hit RTNL between 20 and 100 times per second.  I do not
>see how this is acceptable.
>
>	I believe the proper solution here is to supplant the periodic
>miimon polling from bonding with link state detection based on notifiers
>(As Stephen suggested, not for the first time).
>
>	My suggestion is to have bonding set slave link state based on
>notifiers if miimon is set to zero, and poll as usual if it is not.
>This would preserve any backwards compatibility with any device out
>there that might possibly still be doing netif_carrier_on/off
>incorrectly or not at all.  The only minor complication is synchronizing
>notifier carrier state detection with the ARP monitor.
>
>	This should have been done a long time ago; I'll work something
>up tomorrow (it's late here right now) and post a patch for testing.

That would be awesome. Looking forward to it.

Thanks,
Emil

--
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