[PATCH net-next 0/2] bonding: fix 3ad slave (de)init

[PATCH net-next 0/2] bonding: fix 3ad slave (de)init

[Veaceslav Falico]

After 1f718f0f4f97145f4072d2d72dcf85069ca7226d ("bonding: populate
neighbour's private on enslave") the (de)linking of slaves in
bond_enslave/bond_release_one happens in the correct places - after we've
completely initialized the slave (for bond_enslave) and before we've even
began to de-init the slave (for bond_release_one, respectively).

This was done to prevent any RCU readers to see the half-initialized slave
(because the RCU readers aren't blocked by bond->lock or rtnl_lock
usually).

However, 802.3ad logic, in several places, relied on the fact that the
slave is still linked to the bond.

Fix it by correctly handling these cases - we shouldn't rely that the slave
is linked before fully initialized and, respectively, that the slave is
still linked while it's being removed.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

---
 drivers/net/bonding/bond_3ad.c  | 4 +++-
 drivers/net/bonding/bond_main.c | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

[PATCH net-next 1/2] bonding: correctly verify for the first slave in bond_enslave

[Veaceslav Falico]

After commit 1f718f0f4f97145f4072d2d72dcf85069ca7226d ("bonding: populate
neighbour's private on enslave"), we've moved the actual 'linking' in the
end of the function - so that, once linked, the slave is ready to be used,
and is not still in the process of enslaving.

However, 802.3ad verified if it's the first slave by looking at the

if (bond_first_slave(bond) == new_slave)

which, because we've moved the linking to the end, became broken - on the
first slave bond_first_slave(bond) returns NULL.

Fix this by verifying if the prev_slave, that equals bond_last_slave(), is
actually populated - if it is - then it's not the first slave, and vice
versa.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index d5c3153..0367f80 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1557,7 +1557,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		 */
 		bond_set_slave_inactive_flags(new_slave);
 		/* if this is the first slave */
-		if (bond_first_slave(bond) == new_slave) {
+		if (!prev_slave) {
 			SLAVE_AD_INFO(new_slave).id = 1;
 			/* Initialize AD with the number of times that the AD timer is called in 1 second
 			 * can be called only after the mac address of the bond is set
-- 
1.8.4

[PATCH net-next 2/2] bonding: verify if we still have slaves in bond_3ad_unbind_slave()

[Veaceslav Falico]

After commit 1f718f0f4f97145f4072d2d72dcf85069ca7226d ("bonding: populate
neighbour's private on enslave"), we've moved the unlinking of the slave
to the earliest position possible - so that nobody will see an
half-uninited slave.

However, bond_3ad_unbind_slave() relied that, even while removing the last
slave, it is still accessible - via __get_first_agg() (and, eventually,
bond_first_slave()).

Fix that by verifying if the aggregator return is an actual aggregator, but
not NULL.

CC: Jay Vosburgh <fubar@us.ibm.com>
CC: Andy Gospodarek <andy@greyhouse.net>
Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
---
 drivers/net/bonding/bond_3ad.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 1337eaf..2715ea8 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -2056,7 +2056,9 @@ void bond_3ad_unbind_slave(struct slave *slave)
 				pr_info("%s: Removing an active aggregator\n",
 					slave->bond->dev->name);
 				// select new active aggregator
-				ad_agg_selection_logic(__get_first_agg(port));
+				temp_aggregator = __get_first_agg(port);
+				if (temp_aggregator)
+					ad_agg_selection_logic(temp_aggregator);
 			}
 		}
 	}
-- 
1.8.4

Re: [PATCH net-next 0/2] bonding: fix 3ad slave (de)init

[David Miller]

From: Veaceslav Falico <vfalico@redhat.com>
Date: Fri, 27 Sep 2013 15:10:56 +0200

> After 1f718f0f4f97145f4072d2d72dcf85069ca7226d ("bonding: populate
> neighbour's private on enslave") the (de)linking of slaves in
> bond_enslave/bond_release_one happens in the correct places - after we've
> completely initialized the slave (for bond_enslave) and before we've even
> began to de-init the slave (for bond_release_one, respectively).
> 
> This was done to prevent any RCU readers to see the half-initialized slave
> (because the RCU readers aren't blocked by bond->lock or rtnl_lock
> usually).
> 
> However, 802.3ad logic, in several places, relied on the fact that the
> slave is still linked to the bond.
> 
> Fix it by correctly handling these cases - we shouldn't rely that the slave
> is linked before fully initialized and, respectively, that the slave is
> still linked while it's being removed.
> 
> CC: Jay Vosburgh <fubar@us.ibm.com>
> CC: Andy Gospodarek <andy@greyhouse.net>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>

Series applied.