Re: [net-next] bonding: don't allow the master to become its slave

[Jay Vosburgh <fubar@us.ibm.com>]

Ben Hutchings <bhutchings@solarflare.com> wrote:

>On Thu, 2012-08-09 at 15:30 -0300, Flavio Leitner wrote:
>> It doesn't make any sense to allow the master to become
>> its slave. That creates a loop of events causing a crash.
>
>What if there are other intermediate devices, e.g. the slave is a VLAN
>sub-device of the bond?  And doesn't team also have this problem?
>
>I think a more general check for such loops might be required.

	I thought we had disallowed any nesting of bonds at all, but I
checked the netdev archives, and it appears we discussed it (and agreed
it didn't work), but it kind of petered out.

http://patchwork.ozlabs.org/patch/79705/

	In any event, I think a patch like the following would get all
cases (double enslavement or enslavement of any bonding master) in one
place:

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 6fae5f3..d14651c 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1505,18 +1505,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	int link_reporting;
 	int res = 0;
 
+	if (slave_dev->priv_flags & IFF_BONDING) {
+		pr_debug("Error, Device was already enslaved\n");
+		return -EBUSY;
+	}
+
 	if (!bond->params.use_carrier && slave_dev->ethtool_ops == NULL &&
 		slave_ops->ndo_do_ioctl == NULL) {
 		pr_warning("%s: Warning: no link monitoring support for %s\n",
 			   bond_dev->name, slave_dev->name);
 	}
 
-	/* already enslaved */
-	if (slave_dev->flags & IFF_SLAVE) {
-		pr_debug("Error, Device was already enslaved\n");
-		return -EBUSY;
-	}
-
 	/* vlan challenged mutual exclusion */
 	/* no need to lock since we're protected by rtnl_lock */
 	if (slave_dev->features & NETIF_F_VLAN_CHALLENGED) {


	This is basically the same logic that Jiri Bohac originally
proposed in the discussion I mention above, although this patch moves
the test further up and combines the master and slave tests into one.

	Comments?  I haven't tested this at all, but I think the logic
is correct.  I don't think having two separate tests to get special
"master" and "slave" error cases is worthwhile.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com