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]

Message-ID: <1344546593.2593.24.camel@bwh-desktop.uk.solarflarecom.com>
Date:	Thu, 9 Aug 2012 22:09:53 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Jiri Pirko <jpirko@...hat.com>
CC:	Flavio Leitner <fbl@...hat.com>, netdev <netdev@...r.kernel.org>,
	Jay Vosburgh <fubar@...ibm.com>,
	Andy Gospodarek <andy@...yhouse.net>,
	Leonardo Chiquitto <lchiquitto@...e.com>
Subject: Re: [net-next] bonding: don't allow the master to become its slave

On Thu, 2012-08-09 at 21:55 +0200, Jiri Pirko wrote:
> Thu, Aug 09, 2012 at 09:39:06PM CEST, fbl@...hat.com wrote:
> >On Thu, 9 Aug 2012 20:03:23 +0100
> >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.
> >
> >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:

--- 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.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
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