Subject: Re: [PATCH net] bonding: fix a div error caused by the slave release path

From: Veaceslav Falico <[email-masked]>
Date: Wed, 26 Feb 2014 14:28:54 +0100

On Wed, Feb 26, 2014 at 02:20:30PM +0100, Nikolay Aleksandrov wrote:
>There's a bug in the slave release function which leads the transmit
>functions which use the bond->slave_cnt to a div by 0 because we might
>just have released our last slave and made slave_cnt == 0 but at the same
>time we may have a transmitter after the check for an empty list which will
>fetch it and use it in the slave id calculation.
>Fix it by moving the slave_cnt after synchronize_rcu so if this was our
>last slave any new transmitters will see an empty slave list which is
>checked after rcu lock but before calling the mode transmit functions
>which rely on bond->slave_cnt.
>
>Fixes: 278b208375 ("bonding: initial RCU conversion")
>
>CC: Veaceslav Falico <[email-masked]>
>CC: Andy Gospodarek <[email-masked]>
>CC: Jay Vosburgh <[email-masked]>
>CC: David S. Miller <[email-masked]>
>
>Signed-off-by: Nikolay Aleksandrov <[email-masked]>

Acked-by: Veaceslav Falico <[email-masked]>

On a side note - slave_cnt is used only for roundrobin/xor slave
identification. It *might* be a good idea to remove it completely, though I
can't figure out a way how to remove it without some overhead in fast
path...

>---
> drivers/net/bonding/bond_main.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 1c6104d..5a66094 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -1654,9 +1654,6 @@ static int __bond_release_one(struct net_device *bond_dev,
> 		return -EINVAL;
> 	}
>
>-	/* release the slave from its bond */
>-	bond->slave_cnt--;
>-
> 	bond_sysfs_slave_del(slave);
>
> 	bond_upper_dev_unlink(bond_dev, slave_dev);
>@@ -1738,6 +1735,7 @@ static int __bond_release_one(struct net_device *bond_dev,
>
> 	unblock_netpoll_tx();
> 	synchronize_rcu();
>+	bond->slave_cnt--;
>
> 	if (!bond_has_slaves(bond)) {
> 		call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
>-- 
>1.8.4.2
>