diff mbox

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

Message ID 1344546593.2593.24.camel@bwh-desktop.uk.solarflarecom.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Ben Hutchings Aug. 9, 2012, 9:09 p.m. UTC 
On Thu, 2012-08-09 at 21:55 +0200, Jiri Pirko wrote:
> Thu, Aug 09, 2012 at 09:39:06PM CEST, fbl@redhat.com wrote:
> >On Thu, 9 Aug 2012 20:03:23 +0100
> >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.
> >
> >Maybe patching netdev_set_master() to fail in the loop case is
> >the way to go.  That would work for bonding, team and bridge.
> >
> >What you think?
> 
> 
> How about other devices who do not use "->master" like vlan, macvlan?

And they shouldn't use master, because they allow multiple upper devices
may be stacked on a single lower device.  Instead they use iflink, but
that's an ifindex and not a net_device pointer.

So I think we can catch simple loops with:


But then there can be quite silly device relationships like:

               +-------+
               | bond0 |
               ++-----++
               /       \
+-------+ +---+---+ +---+---+ +-------+
| vlan0 | | vlan1 | | vlan2 | | vlan3 |
+---+---+ +---+---+ +---+---+ +---+---+
     \       /           \       /
     ++-----++           ++--+--++
     | bond1 |           | bond2 |
     +-------+           +-------+
      :     :             :     :

Suppose the user tries to make bond0 a slave of bond1; we need to go to
somewhat more effort to detect the loop.

Ben.

Comments

Jay Vosburgh Aug. 9, 2012, 9:27 p.m. UTC | #1 
Ben Hutchings <bhutchings@solarflare.com> wrote:
>On Thu, 2012-08-09 at 21:55 +0200, Jiri Pirko wrote:
[...]
>> How about other devices who do not use "->master" like vlan, macvlan?
>
>And they shouldn't use master, because they allow multiple upper devices
>may be stacked on a single lower device.  Instead they use iflink, but
>that's an ifindex and not a net_device pointer.
>
>So I think we can catch simple loops with:
>
>--- a/net/core/dev.c
>+++ b/net/core/dev.c
>@@ -4445,8 +4445,22 @@ int netdev_set_master(struct net_device *slave, struct net_device *master)
> 	ASSERT_RTNL();
>
> 	if (master) {
>+		struct net_device *bottom, *top;
>+
> 		if (old)
> 			return -EBUSY;
>+
>+		/* Prevent loops */
>+		bottom = slave;
>+		while (bottom->iflink != bottom->ifindex)
>+			bottom = __dev_get_by_index(dev_net(bottom),
>+						    bottom->iflink);
>+		top = master;
>+		while (top->master)
>+			top = top->master;
>+		if (top == bottom)
>+			return -EBUSY;
>+
> 		dev_hold(master);
> 	}
>
>--- END ---
>
>But then there can be quite silly device relationships like:
>
>               +-------+
>               | bond0 |
>               ++-----++
>               /       \
>+-------+ +---+---+ +---+---+ +-------+
>| vlan0 | | vlan1 | | vlan2 | | vlan3 |
>+---+---+ +---+---+ +---+---+ +---+---+
>     \       /           \       /
>     ++-----++           ++--+--++
>     | bond1 |           | bond2 |
>     +-------+           +-------+
>      :     :             :     :
>
>Suppose the user tries to make bond0 a slave of bond1; we need to go to
>somewhat more effort to detect the loop.

	If that's hard to do (and it might be; I'm not aware of a
standard way to run up and down those stacks of interfaces, which might
not always be vlans in the middle), there's still the priv_flags &
IFF_BONDING test that bonding could (and probably should) do itself as
well.  The team driver could presumably have a similar test, although I
seem to recall that team was allowed to nest.

	FWIW, I've seen both the top and bottom halves of that picture
in use (i.e., bonds consisting of vlans as slaves or bonds with vlans
configured above them), but not combined as in your diagram.

	-J

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Aug. 9, 2012, 11:43 p.m. UTC | #2 
From: Jay Vosburgh <fubar@us.ibm.com>
Date: Thu, 09 Aug 2012 14:27:08 -0700

> 	If that's hard to do (and it might be; I'm not aware of a
> standard way to run up and down those stacks of interfaces, which might
> not always be vlans in the middle), there's still the priv_flags &
> IFF_BONDING test that bonding could (and probably should) do itself as
> well.  The team driver could presumably have a similar test, although I
> seem to recall that team was allowed to nest.
> 
> 	FWIW, I've seen both the top and bottom halves of that picture
> in use (i.e., bonds consisting of vlans as slaves or bonds with vlans
> configured above them), but not combined as in your diagram.

We're basically looking for cycles in a complex graph.

Some combination of Jay and Ben's most recent patches, with some minor
modifications, ought to do it.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiri Pirko Aug. 10, 2012, 1:04 p.m. UTC | #3 
Fri, Aug 10, 2012 at 01:43:31AM CEST, davem@davemloft.net wrote:
>From: Jay Vosburgh <fubar@us.ibm.com>
>Date: Thu, 09 Aug 2012 14:27:08 -0700
>
>> 	If that's hard to do (and it might be; I'm not aware of a
>> standard way to run up and down those stacks of interfaces, which might
>> not always be vlans in the middle), there's still the priv_flags &
>> IFF_BONDING test that bonding could (and probably should) do itself as
>> well.  The team driver could presumably have a similar test, although I
>> seem to recall that team was allowed to nest.
>> 
>> 	FWIW, I've seen both the top and bottom halves of that picture
>> in use (i.e., bonds consisting of vlans as slaves or bonds with vlans
>> configured above them), but not combined as in your diagram.
>
>We're basically looking for cycles in a complex graph.
>
>Some combination of Jay and Ben's most recent patches, with some minor
>modifications, ought to do it.


Hmm. Would be probably good to have list/table of related devices,
possibly with information about the relation.
After that, every relation add would check for loops.

I will dive in the code over the weekend to see if this is doable in
some nice way.

>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4445,8 +4445,22 @@  int netdev_set_master(struct net_device *slave, struct net_device *master)
 	ASSERT_RTNL();
 
 	if (master) {
+		struct net_device *bottom, *top;
+
 		if (old)
 			return -EBUSY;
+
+		/* Prevent loops */
+		bottom = slave;
+		while (bottom->iflink != bottom->ifindex)
+			bottom = __dev_get_by_index(dev_net(bottom),
+						    bottom->iflink);
+		top = master;
+		while (top->master)
+			top = top->master;
+		if (top == bottom)
+			return -EBUSY;
+
 		dev_hold(master);
 	}
 
--- END ---