netdev - Re: [PATCH v2 net] bonding: don't use stale speed and duplex information

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: <7949.1456724392@famine>
Date:	Sun, 28 Feb 2016 21:39:52 -0800
From:	Jay Vosburgh <jay.vosburgh@...onical.com>
To:	zhuyj <zyjzyj2000@...il.com>
cc:	netdev@...r.kernel.org,
	"Tantilov, Emil S" <emil.s.tantilov@...el.com>,
	Veaceslav Falico <vfalico@...il.com>,
	dingtianhong <dingtianhong@...wei.com>,
	Andy Gospodarek <gospo@...ulusnetworks.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2 net] bonding: don't use stale speed and duplex information

zhuyj <zyjzyj2000@...il.com> wrote:

>On 02/25/2016 09:33 PM, Jay Vosburgh wrote:
>> zhuyj <zyjzyj2000@...il.com> wrote:
>> [...]
>>> I delved into the source code and Emil's tests. I think that the problem
>>> that this patch expects to fix occurs very unusually.
>>>
>>> Do you agree with me?
>>>
>>> If so, maybe the following patch can reduce the performance loss.
>>> Please comment on it. Thanks a lot.
>>>
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c
>>> b/drivers/net/bonding/bond_main.c
>>> index b7f1a99..c4c511a 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -2129,7 +2129,9 @@ static void bond_miimon_commit(struct bonding *bond)
>>>                         continue;
>>>
>>>                 case BOND_LINK_UP:
>>> -                       bond_update_speed_duplex(slave);
>>> +                       if (slave->speed == SPEED_UNKNOWN)
>>> +                               bond_update_speed_duplex(slave);
>>> +
>>>                         bond_set_slave_link_state(slave, BOND_LINK_UP,
>>> BOND_SLAVE_NOTIFY_NOW);
>>>                         slave->last_link_up = jiffies;
>> 	I don't believe the speed is necessarily SPEED_UNKNOWN coming in
>> here.  If the race occurs at a time later than the initial enslavement,
>> speed may already be set (and the race manifests if the new speed
>> changes, i.e., the link changes from 1 Gb/sec to 10 Gb/sec), so I don't
>> think this is functionally correct.
>Hi, Jay
>
>Thanks for your reply.
>
>IMHO, "If the race occurs at a time later than the initial enslavement,
>speed may already be set (and the race manifests if the new speed
>changes, i.e., the link changes from 1 Gb/sec to 10 Gb/sec)", from my test,
>this will not happen because the previous source code make the speed
>correct.

	How, exactly, will "the previous source code make the speed
correct"?

>This "bond_update_speed_duplex" repeats to get the correct speed.
>
>That is, this patch is to fix the error in initial enslavement. The
>mentioned scenario will not occur.

	I see nothing in the code that limits the race to happening only
at enslavement time.

	If the bond_mii_monitor call executes between the device going
link up and the arrival of the NETDEV_CHANGE or NETDEV_UP callback, the
stored speed and duplex are stale.  The stale speed value is not
guaranteed to be SPEED_UNKNOWN, so your patch is not functionally
correct.

	-J

>Even though the performance impact is minimal, if we can avoid this
>performance
>impact, why not ?
>
>Best Regards!
>Zhu Yanjun
>
>>
>> 	Also, the call to bond_miimon_commit itself is already gated by
>> bond_miimon_inspect finding a link state change.  The performance impact
>> here should be minimal.
>>
>> 	-J

---
	-Jay Vosburgh, jay.vosburgh@...onical.com