netdev - Re: [PATCH net] bonding: fix div by zero while enslaving and transmitting

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: <1410527381.7106.81.camel@edumazet-glaptop2.roam.corp.google.com>
Date:	Fri, 12 Sep 2014 06:09:41 -0700
From:	Eric Dumazet <eric.dumazet@...il.com>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
Cc:	netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
	Andy Gospodarek <andy@...yhouse.net>,
	Jay Vosburgh <j.vosburgh@...il.com>,
	Veaceslav Falico <vfalico@...il.com>
Subject: Re: [PATCH net] bonding: fix div by zero while enslaving and
 transmitting

On Fri, 2014-09-12 at 14:22 +0200, Nikolay Aleksandrov wrote:
...
> 
> CC: Jiri Pirko <jiri@...nulli.us>
> CC: Andy Gospodarek <andy@...yhouse.net>
> CC: Jay Vosburgh <j.vosburgh@...il.com>
> CC: Veaceslav Falico <vfalico@...il.com>
> Fixes: 5378c2e6ea236d ("bonding: move bond-specific init after enslave happens")
> Signed-off-by: Nikolay Aleksandrov <nikolay@...hat.com>
> ---
>  drivers/net/bonding/bond_main.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 57912ee231cb..10ad434ea184 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1552,6 +1552,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  		goto err_detach;
>  	}
>  
> +	/* Increment slave_cnt before linking in the slave so we won't end up in
> +	 * bond_start_xmit with bond_has_slaves() true and slave_cnt == 0.
> +	 */
> +	bond->slave_cnt++;

It looks like explicit barriers are missing.

#define bond_has_slaves(bond) !list_empty(bond_slave_list(bond)) 

So your increment into slave_cnt must be committed into memory before
any change to slave_list. But you need to check how removal of a slave
is handled.

Now I wonder why bond_has_slaves(bond) is not a test against
bond->slave_cnt

Note that even if this would be the case, bond xmit seems racy :

if (bond_has_slaves(bond))
	ret = __bond_start_xmit(skb, dev);

As slave_cnt could change (and eventually reach 0) between the two
places.

My feeling is that RCU conversion is not properly done in this driver.

Either bond->slave_cnt should be read _once_ for the whole duration of
bond_start_xmit() call, _OR_, be stored in a real Read Copy structure,
so that struct->slave_cnt _cannot_ change during bond_start_xmit()

>  	res = bond_master_upper_dev_link(bond_dev, slave_dev, new_slave);
>  	if (res) {
>  		netdev_dbg(bond_dev, "Error %d calling bond_master_upper_dev_link\n", res);
> @@ -1564,7 +1568,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>  		goto err_upper_unlink;
>  	}
>  
> -	bond->slave_cnt++;
>  	bond_compute_features(bond);
>  	bond_set_carrier(bond);
>  
> @@ -1590,6 +1593,7 @@ err_upper_unlink:
>  
>  err_unregister:
>  	netdev_rx_handler_unregister(slave_dev);
> +	bond->slave_cnt--;
>  
>  err_detach:
>  	if (!bond_uses_primary(bond))


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