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

[Nikolay Aleksandrov <nikolay@redhat.com>]

On 09/12/2014 03:09 PM, Eric Dumazet wrote:
> On Fri, 2014-09-12 at 14:22 +0200, Nikolay Aleksandrov wrote:
> ...
>>
>> CC: Jiri Pirko <jiri@resnulli.us>
>> CC: Andy Gospodarek <andy@greyhouse.net>
>> CC: Jay Vosburgh <j.vosburgh@gmail.com>
>> CC: Veaceslav Falico <vfalico@gmail.com>
>> Fixes: 5378c2e6ea236d ("bonding: move bond-specific init after enslave happens")
>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.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.
> 
That is handled by decrementing slave_cnt after executing synchronize_rcu()
after unlinking the last slave thus making the list empty and all xmitters
entering will see bond_has_slaves() as empty before they see slave_cnt as 0.
In every other case the worst that could happen is that a few packets will
see wrong slave_cnt, but that is not a problem since we walk the list to
find the slave with the id.

> Now I wonder why bond_has_slaves(bond) is not a test against
> bond->slave_cnt
> 
It used to be once, I don't remember the reason it's not anymore.

> Note that even if this would be the case, bond xmit seems racy :
> 
> if (bond_has_slaves(bond))
> 	ret = __bond_start_xmit(skb, dev);
> 
Yes, true but we make sure it doesn't see slave_cnt as 0 with
bond_has_slaves() evaluating to true.

> As slave_cnt could change (and eventually reach 0) between the two
> places.
This shouldn't be possible because of the synchronize_rcu() after unlinking
the slave. slave_cnt is decremented only after that so every reader will
see the list empty before they see slave_cnt as 0.

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