netdev - Re: [PATCH next] bonding: speed/duplex update at NETDEV

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:   Fri, 29 Sep 2017 15:02:16 -0700
From:   Mahesh Bandewar (महेश बंडेवार) 
        <maheshb@...gle.com>
To:     Stephen Hemminger <stephen@...workplumber.org>
Cc:     Mahesh Bandewar <mahesh@...dewar.net>,
        Jay Vosburgh <j.vosburgh@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        Veaceslav Falico <vfalico@...il.com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH next] bonding: speed/duplex update at NETDEV_UP event

On Fri, Sep 29, 2017 at 1:08 PM, Stephen Hemminger
<stephen@...workplumber.org> wrote:
> On Wed, 27 Sep 2017 18:03:49 -0700
> Mahesh Bandewar <mahesh@...dewar.net> wrote:
>
>> From: Mahesh Bandewar <maheshb@...gle.com>
>>
>> Some NIC drivers don't have correct speed/duplex settings at the
>> time they send NETDEV_UP notification and that messes up the
>> bonding state. Especially 802.3ad mode which is very sensitive
>> to these settings. In the current implementation we invoke
>> bond_update_speed_duplex() when we receive NETDEV_UP, however,
>> ignore the return value. If the values we get are invalid
>> (UNKNOWN), then slave gets removed from the aggregator with
>> speed and duplex set to UNKNOWN while link is still marked as UP.
>>
>> This patch fixes this scenario. Also 802.3ad mode is sensitive to
>> these conditions while other modes are not, so making sure that it
>> doesn't change the behavior for other modes.
>>
>> Signed-off-by: Mahesh Bandewar <maheshb@...gle.com>
>> ---
>>  drivers/net/bonding/bond_main.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index b7313c1d9dcd..177be373966b 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -3076,7 +3076,16 @@ static int bond_slave_netdev_event(unsigned long event,
>>               break;
>>       case NETDEV_UP:
>>       case NETDEV_CHANGE:
>> -             bond_update_speed_duplex(slave);
>> +             /* For 802.3ad mode only:
>> +              * Getting invalid Speed/Duplex values here will put slave
>> +              * in weird state. So mark it as link-down for the time
>> +              * being and let link-monitoring (miimon) set it right when
>> +              * correct speeds/duplex are available.
>> +              */
>> +             if (bond_update_speed_duplex(slave) &&
>> +                 BOND_MODE(bond) == BOND_MODE_8023AD)
>> +                     slave->link = BOND_LINK_DOWN;
>> +
>>               if (BOND_MODE(bond) == BOND_MODE_8023AD)
>>                       bond_3ad_adapter_speed_duplex_changed(slave);
>>               /* Fallthrough */
>
> Then fix the drivers. Trying to workaround it here isn't helping.
>
This is not a workaround. It avoids bonding state being weird.

> The problem is that miimon is not required.  Bonding can be purely
> event driven.
>
really? Here is a code-snippet from bonding itself -

        /* reset values for 802.3ad/TLB/ALB */
        if (!bond_mode_uses_arp(bond_mode)) {
                if (!miimon) {
                        pr_warn("Warning: miimon must be specified,
otherwise bonding will not detect link failure, speed and duplex which
are essential for 802.3ad operation\n");
                        pr_warn("Forcing miimon to 100msec\n");
                        miimon = BOND_DEFAULT_MIIMON;
                }
        }