[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <21070.1344540211@death.nxdomain>
Date: Thu, 09 Aug 2012 12:23:31 -0700
From: Jay Vosburgh <fubar@...ibm.com>
To: Ben Hutchings <bhutchings@...arflare.com>
cc: Flavio Leitner <fbl@...hat.com>, netdev <netdev@...r.kernel.org>,
Andy Gospodarek <andy@...yhouse.net>,
Leonardo Chiquitto <lchiquitto@...e.com>,
Jiri Pirko <jpirko@...hat.com>
Subject: Re: [net-next] bonding: don't allow the master to become its slave
Ben Hutchings <bhutchings@...arflare.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@...ibm.com
--
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
Powered by blists - more mailing lists