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

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: <20140226132854.GA24011@redhat.com>
Date:	Wed, 26 Feb 2014 14:28:54 +0100
From:	Veaceslav Falico <vfalico@...hat.com>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
Cc:	netdev@...r.kernel.org, Andy Gospodarek <andy@...yhouse.net>,
	Jay Vosburgh <fubar@...ibm.com>,
	"David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH net] bonding: fix a div error caused by the slave release
 path

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 <vfalico@...hat.com>
>CC: Andy Gospodarek <andy@...yhouse.net>
>CC: Jay Vosburgh <fubar@...ibm.com>
>CC: David S. Miller <davem@...emloft.net>
>
>Signed-off-by: Nikolay Aleksandrov <nikolay@...hat.com>

Acked-by: Veaceslav Falico <vfalico@...hat.com>

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