bonding: ban stacked bonding support

PATCH Superseded Alexey Dobriyan 5 participants 9 messages Feb 20, 2015 – Mar 20, 2015

Subsystems (3)

Networking Network drivers Bonding driver

lore

Share & tools

lore

Alexey Dobriyan

author

2015-02-20 22:20 UTC #0 lore raw

PATCH 1/1 bonding: ban stacked bonding support

Does Linux support it at all?

In short: if you add bonding master as a slave, and then release it, it will no longer be a IFF_BONDING creating problems like described at
https://bugzilla.kernel.org/show_bug.cgi?id=89541

echo +bond1 >/sys/class/net/bonding_masters echo 1 >/sys/class/net/bond1/bonding/mode echo +bond2 >/sys/class/net/bonding_masters echo +bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bonding_masters

cat /proc/net/bonding/bond2 # should not exist
[oops]

Signed-off-by Alexey Dobriyan <[email protected]>

---

 drivers/net/bonding/bond_main.c |    5 +++++
 1 file changed, 5 insertions(+)

drivers/net/bonding/bond_main.c +5 -0

@@ -1248,6 +1248,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 			    slave_dev->name);
 	}
 
+	if (slave_dev->flags & IFF_MASTER) {
+		netdev_dbg(bond_dev, "stacked bonding not supported\n");
+		return -EBUSY;
+	}
+
 	/* already enslaved */
 	if (slave_dev->flags & IFF_SLAVE) {
 		netdev_dbg(bond_dev, "Error: Device was already enslaved\n");

Jay Vosburgh

2015-02-20 23:14 UTC #1 lore raw

Quoted 13 lines · Alexey Dobriyan

Does Linux support it at all?
In short: if you add bonding master as a slave, and then release it, it will no longer be a IFF_BONDING creating problems like described at
https://bugzilla.kernel.org/show_bug.cgi?id=89541
echo +bond1 >/sys/class/net/bonding_masters echo 1 >/sys/class/net/bond1/bonding/mode echo +bond2 >/sys/class/net/bonding_masters echo +bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bonding_masters

Quoted 4 lines

cat /proc/net/bonding/bond2 # should not exist
[oops]
Signed-off-by Alexey Dobriyan <[email protected]>

I think it's time to disallow stacking like this; it never
really worked quite right as far as I can remember, and I thought it was disallowed at some point in the past. I don't believe the stacked bonds function correctly for receive in the current kernel, either, although I'd have to test it again to confirm that.

The usual case for desiring to stack bonds is an active-backup
pair of LACP / 802.3ad bonds (such as the bugzilla referenced above), but the 802.3ad mode handles this situation internally, so no stack is necessary.

Quoted 9 lines

---
drivers/net/bonding/bond_main.c | 5 +++++
1 file changed, 5 insertions(+)
drivers/net/bonding/bond_main.c +0 -0
@@ -1248,6 +1248,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 			    slave_dev->name);

drivers/net/bonding/bond_main.c int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) · line 1258

Quoted 10 lines

drivers/net/bonding/bond_main.c +5 -0

 	}
 
+	if (slave_dev->flags & IFF_MASTER) {
+		netdev_dbg(bond_dev, "stacked bonding not supported\n");
+		return -EBUSY;
+	}
+
 	/* already enslaved */
 	if (slave_dev->flags & IFF_SLAVE) {
 		netdev_dbg(bond_dev, "Error: Device was already enslaved\n");

Instead of a separate block for IFF_MASTER, the IFF_SLAVE line
could be replaced with something like:

	if (netif_is_bond_slave(slave_dev) || netif_is_bond_master(slave_dev)) {
		netdev_dbg(bond_dev, "Error: Device is bond slave or master\n");

	With that caveat:

Signed-off-by Jay Vosburgh <[email protected]>

	This is probably a good candidate for -stable as well.

	-J

---
-Jay Vosburgh, [email protected]

Andy Gospodarek

2015-02-21 02:55 UTC #2 lore raw

Quoted 26 lines · Jay Vosburgh

Quoted 13 lines · Alexey Dobriyan
Does Linux support it at all?
In short: if you add bonding master as a slave, and then release it, it will no longer be a IFF_BONDING creating problems like described at
https://bugzilla.kernel.org/show_bug.cgi?id=89541
echo +bond1 >/sys/class/net/bonding_masters echo 1 >/sys/class/net/bond1/bonding/mode echo +bond2 >/sys/class/net/bonding_masters echo +bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bonding_masters
Quoted 4 lines
cat /proc/net/bonding/bond2 # should not exist
[oops]
Signed-off-by Alexey Dobriyan <[email protected]>
I think it's time to disallow stacking like this; it never
really worked quite right as far as I can remember, and I thought it was disallowed at some point in the past. I don't believe the stacked bonds function correctly for receive in the current kernel, either, although I'd have to test it again to confirm that.

Quoted 4 lines

The usual case for desiring to stack bonds is an active-backup
pair of LACP / 802.3ad bonds (such as the bugzilla referenced above), but the 802.3ad mode handles this situation internally, so no stack is necessary.

Completely agree.

Quoted 24 lines

Quoted 10 lines
---
drivers/net/bonding/bond_main.c | 5 +++++
1 file changed, 5 insertions(+)
drivers/net/bonding/bond_main.c +0 -0
@@ -1248,6 +1248,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 			    slave_dev->name);
drivers/net/bonding/bond_main.c int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) · line 1258
Quoted 10 lines
drivers/net/bonding/bond_main.c +5 -0
 	}
 
+	if (slave_dev->flags & IFF_MASTER) {
+		netdev_dbg(bond_dev, "stacked bonding not supported\n");
+		return -EBUSY;
+	}
+
 	/* already enslaved */
 	if (slave_dev->flags & IFF_SLAVE) {
 		netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
Instead of a separate block for IFF_MASTER, the IFF_SLAVE line
could be replaced with something like:

Quoted 4 lines

	if (netif_is_bond_slave(slave_dev) || netif_is_bond_master(slave_dev)) {
		netdev_dbg(bond_dev, "Error: Device is bond slave or master\n");

	With that caveat:

I would say adding the check for netif_is_bond_master() is critical since a check for only IFF_MASTER would fail when trying to add a bridge to a bond and that failure would not be desireable.

Fix that and you can also add:

Signed-off-by Andy Gospodarek <[email protected]>

to your next post.

Quoted 4 lines

Signed-off-by Jay Vosburgh <[email protected]>
This is probably a good candidate for -stable as well.

Agree

(Though netif_is_bond_master() was added in 3.10; luckily it would be an easy backport.)

Quoted 9 lines

	-J
---
-Jay Vosburgh, [email protected]
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jiri Pirko

2015-02-21 16:59 UTC #3 lore raw

In reply to Jay Vosburgh

Quoted 26 lines · Sat, Feb 21, 2015 at 12:14:00AM CET, [email protected] wrote:

Quoted 13 lines · Alexey Dobriyan
Does Linux support it at all?
In short: if you add bonding master as a slave, and then release it, it will no longer be a IFF_BONDING creating problems like described at
https://bugzilla.kernel.org/show_bug.cgi?id=89541
echo +bond1 >/sys/class/net/bonding_masters echo 1 >/sys/class/net/bond1/bonding/mode echo +bond2 >/sys/class/net/bonding_masters echo +bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bonding_masters
Quoted 4 lines
cat /proc/net/bonding/bond2 # should not exist
[oops]
Signed-off-by Alexey Dobriyan <[email protected]>
I think it's time to disallow stacking like this; it never
really worked quite right as far as I can remember, and I thought it was disallowed at some point in the past. I don't believe the stacked bonds function correctly for receive in the current kernel, either, although I'd have to test it again to confirm that.

Quoted 4 lines

The usual case for desiring to stack bonds is an active-backup
pair of LACP / 802.3ad bonds (such as the bugzilla referenced above), but the 802.3ad mode handles this situation internally, so no stack is necessary.

Exactly. There is no real use-case for stacked bonding.

Quoted 41 lines

Quoted 10 lines
---
drivers/net/bonding/bond_main.c | 5 +++++
1 file changed, 5 insertions(+)
drivers/net/bonding/bond_main.c +0 -0
@@ -1248,6 +1248,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 			    slave_dev->name);
drivers/net/bonding/bond_main.c int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) · line 1258
Quoted 10 lines
drivers/net/bonding/bond_main.c +5 -0
 	}
 
+	if (slave_dev->flags & IFF_MASTER) {
+		netdev_dbg(bond_dev, "stacked bonding not supported\n");
+		return -EBUSY;
+	}
+
 	/* already enslaved */
 	if (slave_dev->flags & IFF_SLAVE) {
 		netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
Instead of a separate block for IFF_MASTER, the IFF_SLAVE line
could be replaced with something like:
	if (netif_is_bond_slave(slave_dev) || netif_is_bond_master(slave_dev)) {
		netdev_dbg(bond_dev, "Error: Device is bond slave or master\n");

	With that caveat:
Signed-off-by Jay Vosburgh <[email protected]>
	This is probably a good candidate for -stable as well.

	-J
---
-Jay Vosburgh, [email protected]
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alexey Dobriyan

author

2015-03-20 17:43 UTC #4 lore raw

In reply to Jay Vosburgh

Quoted 5 lines · Jay Vosburgh

Instead of a separate block for IFF_MASTER, the IFF_SLAVE line
could be replaced with something like:
	if (netif_is_bond_slave(slave_dev) || netif_is_bond_master(slave_dev)) {
		netdev_dbg(bond_dev, "Error: Device is bond slave or master\n");

I think separate error messages are better (kernel can distinguish between two anyway).

Alexey Dobriyan

author

2015-03-20 17:46 UTC #5 lore raw

PATCH v2 1/1 bonding: ban stacked bonding support

If you add bonding master as a slave, and then release it, it will no longer be an IFF_BONDING creating problems like described at
https://bugzilla.kernel.org/show_bug.cgi?id=89541

cat /proc/net/bonding/bond2 # should not exist
[oops]

Signed-off-by Alexey Dobriyan <[email protected]>

---

 drivers/net/bonding/bond_main.c |    5 +++++
 1 file changed, 5 insertions(+)

drivers/net/bonding/bond_main.c +5 -0

@@ -1248,6 +1248,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 			    slave_dev->name);
 	}
 
+	if (netif_is_bond_master(slave_dev)) {
+		netdev_err(bond_dev, "device is bond master\n");
+		return -EBUSY;
+	}
+
 	/* already enslaved */
 	if (slave_dev->flags & IFF_SLAVE) {
 		netdev_dbg(bond_dev, "Error: Device was already enslaved\n");

David Miller

maintainer

2015-03-20 20:38 UTC #6 lore raw

From: Alexey Dobriyan <[email protected]>
Date: Fri, 20 Mar 2015 20:46:38 +0300

Quoted 11 lines

If you add bonding master as a slave, and then release it, it will no longer be an IFF_BONDING creating problems like described at
https://bugzilla.kernel.org/show_bug.cgi?id=89541
echo +bond1 >/sys/class/net/bonding_masters echo 1 >/sys/class/net/bond1/bonding/mode echo +bond2 >/sys/class/net/bonding_masters echo +bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bonding_masters

Quoted 4 lines

cat /proc/net/bonding/bond2 # should not exist
[oops]
Signed-off-by Alexey Dobriyan <[email protected]>

I feel like this has been brought up before and it was stated that some people are actually using things like this.

I could be mistaken.

Jay Vosburgh

2015-03-20 21:02 UTC #7 lore raw

Quoted 17 lines · David Miller

From: Alexey Dobriyan <[email protected]>
Date: Fri, 20 Mar 2015 20:46:38 +0300
Quoted 14 lines
If you add bonding master as a slave, and then release it, it will no longer be an IFF_BONDING creating problems like described at
https://bugzilla.kernel.org/show_bug.cgi?id=89541
echo +bond1 >/sys/class/net/bonding_masters echo 1 >/sys/class/net/bond1/bonding/mode echo +bond2 >/sys/class/net/bonding_masters echo +bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bonding_masters
cat /proc/net/bonding/bond2 # should not exist
[oops]

Quoted 6 lines

Quoted 1 line
Signed-off-by Alexey Dobriyan <[email protected]>
I feel like this has been brought up before and it was stated that some people are actually using things like this.
I could be mistaken.

I don't think you are. I did a bit of checking after the
discussion last month and found a few relatively recent statements that people were nesting bonds and it was apparently working, e.g.,

http://www.alexwitherspoon.com/debian-nested-bonded-interfaces/

which, ironically, is exactly the case that would benefit from
not nesting the bonds, as 802.3ad would handle multiple aggregators itself.

However, there is also this discussion

http://lists.openwall.net/netdev/2011/01/22/66

from netdev in 2011 that states that the ingress path of nested
bonds does not work, at least for the case described. Perhaps some configurations work and some don't.

Let me see if I can run a quick test and see if this actually
works for me...

-J

---
-Jay Vosburgh, [email protected]

Jay Vosburgh

2015-03-20 22:30 UTC #8 lore raw

Quoted 38 lines · Jay Vosburgh

David Miller <[email protected]> wrote:
From: Alexey Dobriyan <[email protected]>
Quoted 16 lines
Date: Fri, 20 Mar 2015 20:46:38 +0300
Quoted 14 lines
If you add bonding master as a slave, and then release it, it will no longer be an IFF_BONDING creating problems like described at
https://bugzilla.kernel.org/show_bug.cgi?id=89541
echo +bond1 >/sys/class/net/bonding_masters echo 1 >/sys/class/net/bond1/bonding/mode echo +bond2 >/sys/class/net/bonding_masters echo +bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bond1/bonding/slaves echo -bond2 >/sys/class/net/bonding_masters
cat /proc/net/bonding/bond2 # should not exist
[oops]
Quoted 6 lines
Quoted 1 line
Signed-off-by Alexey Dobriyan <[email protected]>
I feel like this has been brought up before and it was stated that some people are actually using things like this.
I could be mistaken.
I don't think you are. I did a bit of checking after the
discussion last month and found a few relatively recent statements that people were nesting bonds and it was apparently working, e.g.,
http://www.alexwitherspoon.com/debian-nested-bonded-interfaces/
which, ironically, is exactly the case that would benefit from
not nesting the bonds, as 802.3ad would handle multiple aggregators itself.
However, there is also this discussion

Quoted 8 lines

http://lists.openwall.net/netdev/2011/01/22/66
from netdev in 2011 that states that the ingress path of nested
bonds does not work, at least for the case described. Perhaps some configurations work and some don't.
Let me see if I can run a quick test and see if this actually
works for me...

I ran a few tests against net-next from a couple of days ago.

A simple test of two balance-rr mode bonds nested below an
active-backup mode bond appears to function, passing traffic in both directions. I didn't test extensively, but ingress does not appear to be broken as the 2011 netdev discussion indicates.

The sequence supplied by Alexey does reveal a bug, in that the
bond2 /proc file isn't removed when it should be. In light of the above, however, this will have to be fixed some way other than disallowing nesting.

-J

---
-Jay Vosburgh, [email protected]