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

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]

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