netdev - Re: [PATCH v2] bonding: start slaves with link down for ARP monitor

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20120415002153.4c4e152a@asterix.rh>
Date:	Sun, 15 Apr 2012 00:21:53 -0300
From:	Flavio Leitner <fbl@...hat.com>
To:	Michal Kubecek <mkubecek@...e.cz>
Cc:	netdev@...r.kernel.org, Jay Vosburgh <fubar@...ibm.com>,
	Andy Gospodarek <andy@...yhouse.net>
Subject: Re: [PATCH v2] bonding: start slaves with link down for ARP monitor

On Sat, 14 Apr 2012 22:16:16 +0200
Michal Kubecek <mkubecek@...e.cz> wrote:

> Initialize slave device link state as down if ARP monitor is
> active and net_carrier_ok() returns zero. Also shift initial
> value of its last_arp_tx so that it doesn't immediately cause
> fake detection of "up" state.
> 
> When ARP monitoring is used, initializing the slave device with
> up link state can cause ARP monitor to detect link failure
> before the device is really up (with igb driver, this can take
> more than two seconds).
> 
> Signed-off-by: Michal Kubecek <mkubecek@...e.cz>
> ---
>  drivers/net/bonding/bond_main.c |   34 +++++++++++++++++++++-------------
>  1 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 62d2409..6a79ee3 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1726,7 +1726,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  
>  	read_lock(&bond->lock);
>  
> -	new_slave->last_arp_rx = jiffies;
> +	new_slave->last_arp_rx = jiffies -
> +		(msecs_to_jiffies(bond->params.arp_interval) + 1);
>  
>  	if (bond->params.miimon && !bond->params.use_carrier) {
>  		link_reporting = bond_check_dev_link(bond, slave_dev, 1);
> @@ -1751,21 +1752,28 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  	}
>  
>  	/* check for initial state */
> -	if (!bond->params.miimon ||
> -	    (bond_check_dev_link(bond, slave_dev, 0) == BMSR_LSTATUS)) {
> -		if (bond->params.updelay) {
> -			pr_debug("Initial state of slave_dev is BOND_LINK_BACK\n");
> -			new_slave->link  = BOND_LINK_BACK;
> -			new_slave->delay = bond->params.updelay;
> +	if (bond->params.miimon) {
> +		if (bond_check_dev_link(bond, slave_dev, 0) == BMSR_LSTATUS) {
> +			if (bond->params.updelay) {
> +				new_slave->link = BOND_LINK_BACK;
> +				new_slave->delay = bond->params.updelay;
> +			} else {
> +				new_slave->link = BOND_LINK_UP;
> +			}
>  		} else {
> -			pr_debug("Initial state of slave_dev is BOND_LINK_UP\n");
> -			new_slave->link  = BOND_LINK_UP;
> +			new_slave->link = BOND_LINK_DOWN;
>  		}
> +	} else if (bond->params.arp_interval) {
> +		new_slave->link = (netif_carrier_ok(slave_dev) ?
> +			BOND_LINK_UP : BOND_LINK_DOWN);

The interface would have to negotiate the link and report back
very very fast because the dev_open(slave) was just called and
most drivers initialize the state as DOWN and then wait either
for an interrupt or a watchdog to update the link status.

Therefore, the practical final result for most cards (if not all)
is new_slave->link = BOND_LINK_DOWN and forced to wait for an
ARP monitor cycle before going link up according to ARP monitor.

This will change the current behavior which is faster and good
enough for most cases.  What about a new option to decide that?
For instance, arp_init_slave=UP/DOWN/MII with default to UP.

Jay? Andy? :)


> +	} else
> +		new_slave->link = BOND_LINK_UP;
> +
> +	if (new_slave->link != BOND_LINK_DOWN)
>  		new_slave->jiffies = jiffies;
> -	} else {
> -		pr_debug("Initial state of slave_dev is BOND_LINK_DOWN\n");
> -		new_slave->link  = BOND_LINK_DOWN;
> -	}
> +	pr_debug("Initial state of slave_dev is BOND_LINK_%s\n",
> +		new_slave->link == BOND_LINK_DOWN ? "DOWN" :
> +			(new_slave->link == BOND_LINK_UP ? "UP" : "BACK"));

The above seems to have missed a 'space' and the alignment of
the next line:
+	pr_debug("Initial state of slave_dev is BOND_LINK_%s\n",
+		 new_slave->link == BOND_LINK_DOWN ? "DOWN" :
+		 (new_slave->link == BOND_LINK_UP ? "UP" : "BACK"));

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