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]

Message-ID: <20170929130819.74879e99@xeon-e3>
Date:   Fri, 29 Sep 2017 13:08:19 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Mahesh Bandewar <mahesh@...dewar.net>
Cc:     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>,
        Mahesh Bandewar <maheshb@...gle.com>
Subject: Re: [PATCH next] bonding: speed/duplex update at NETDEV_UP event

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.

The problem is that miimon is not required.  Bonding can be purely
event driven.