[PATCH] macvlan: Support creating macvlans from macvlans

[PATCH] macvlan: Support creating macvlans from macvlans

[Eric W. Biederman]

When running in a network namespace whose only link to
the outside world is a macvlan device, not being
able to create another macvlan is a real pain.

So modify macvlan creation to allow automatically forward
a creation of a macvlan on a macvlan to become a creation
of a macvlan on the underlying network device.

Signed-off-by: Eric Biederman <ebiederm@aristanetworks.com>
---
 drivers/net/macvlan.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 7e24b50..b5241fc 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -461,12 +461,13 @@ static int macvlan_newlink(struct net_device *dev,
 	if (lowerdev == NULL)
 		return -ENODEV;
 
-	/* Don't allow macvlans on top of other macvlans - its not really
-	 * wrong, but lockdep can't handle it and its not useful for anything
-	 * you couldn't do directly on top of the real device.
+	/* When creating macvlans on top of other macvlans - use
+	 * the real device as the lowerdev.
 	 */
-	if (lowerdev->rtnl_link_ops == dev->rtnl_link_ops)
-		return -ENODEV;
+	if (lowerdev->rtnl_link_ops == dev->rtnl_link_ops) {
+		struct macvlan_dev *lowervlan = netdev_priv(lowerdev);
+		lowerdev = lowervlan->lowerdev;
+	}
 
 	if (!tb[IFLA_MTU])
 		dev->mtu = lowerdev->mtu;
-- 
1.6.1.2.350.g88cc

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[Patrick McHardy]

Eric W. Biederman wrote:
> When running in a network namespace whose only link to
> the outside world is a macvlan device, not being
> able to create another macvlan is a real pain.
> 
> So modify macvlan creation to allow automatically forward
> a creation of a macvlan on a macvlan to become a creation
> of a macvlan on the underlying network device.

I'm not sure I understand the constallation, what is the underlying
device in this case? A device outside the namespace?

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[Eric W. Biederman]

Patrick McHardy <kaber@trash.net> writes:

> Eric W. Biederman wrote:
>> When running in a network namespace whose only link to
>> the outside world is a macvlan device, not being
>> able to create another macvlan is a real pain.
>>
>> So modify macvlan creation to allow automatically forward
>> a creation of a macvlan on a macvlan to become a creation
>> of a macvlan on the underlying network device.
>
> I'm not sure I understand the constallation, what is the underlying
> device in this case? A device outside the namespace?

Yes.

Typical usage would be:

eth0 in the initial namespace.
A macvlan off of eth0 in each child namespace.

Which works fine until I do things like create a network namespace
when I am already inside of a network namespace.  A child of a child.
In which case I have to start rigging up something like a pair of
veths an bridging or routing to get outside connectivity.

Or roughly:
ip link add mv0 link eth0 type macvlan.
ip link add mv1 link eth0 type macvlan.
ip link set mv0 netns 1234
ip link set mv1 netns 6789

Then later I would find it very handy to do:
echo $$ -> 1234
ip link add mv3 link mv0 type macvlan
ip link set mv3 netns 101112

The fact that we only bridge traffic on the ingress from the external
world is also a pain, but that isn't trivial to fix, and fixing
it might possibly break valid macvlan users.

Eric

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[Patrick McHardy]

Eric W. Biederman wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
>>> So modify macvlan creation to allow automatically forward
>>> a creation of a macvlan on a macvlan to become a creation
>>> of a macvlan on the underlying network device.
>> I'm not sure I understand the constallation, what is the underlying
>> device in this case? A device outside the namespace?
> 
> Yes.
> 
> Typical usage would be:
> 
> eth0 in the initial namespace.
> A macvlan off of eth0 in each child namespace.
> 
> Which works fine until I do things like create a network namespace
> when I am already inside of a network namespace.  A child of a child.
> In which case I have to start rigging up something like a pair of
> veths an bridging or routing to get outside connectivity.
> 
> Or roughly:
> ip link add mv0 link eth0 type macvlan.
> ip link add mv1 link eth0 type macvlan.
> ip link set mv0 netns 1234
> ip link set mv1 netns 6789
> 
> Then later I would find it very handy to do:
> echo $$ -> 1234
> ip link add mv3 link mv0 type macvlan
> ip link set mv3 netns 101112

That makes sense of course. I'm mainly wondering whether a namespace
should be able to directly affect the real device like this. This
might move it to promiscous mode, or affect other performce-relevant
settings. Its also looks like you can steal the MAC address of a
different macvlan device this way and have the packets directed to you
(new devices are added to the beginning of the hash chains, so they
are found first on lookups).

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[Eric W. Biederman]

Patrick McHardy <kaber@trash.net> writes:

> That makes sense of course. I'm mainly wondering whether a namespace
> should be able to directly affect the real device like this. This
> might move it to promiscous mode, or affect other performce-relevant
> settings. Its also looks like you can steal the MAC address of a
> different macvlan device this way and have the packets directed to you
> (new devices are added to the beginning of the hash chains, so they
> are found first on lookups).

To a large extent those are things that we already can do, simply by
having multiple mcavlans in different network namespaces.  I could
push it into promiscous mode by adding more multicast listeners,
and I could steal the mac address of another macvlan by changing
my mac address if I happen to come first in the hash chain.

Hmm.  Actually that appears to be a macvlan bug.  It looks like if I
change the macaddress on a macvlan we don't update the hash chains.
So unless we have the same low byte we will be on the wrong hash chain
and not receive the packets for the mac we specified.  Ouch!

It is also trivial to spoof a different macvlan device by using
PF_PACKET and sending packets with the source mac address of
another macvlan.

Also this still requires CAP_NET_ADMIN, as much as I would like
to remove that restriction.

Your concerns don't appear to be new to allowing the creation
of a macvlan from a macvlan or fundamental to creating
a macvlan from a macvlan.  You still must have access to at
least a macvlan in your namespace to create a new one.  So
I don't think those issues bear on my patch.



That said I am not opposed conceptually to something that is much
harder to abuse, and works better for the network namespace case.

Eric

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[Patrick McHardy]

Eric W. Biederman wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
>> That makes sense of course. I'm mainly wondering whether a namespace
>> should be able to directly affect the real device like this. This
>> might move it to promiscous mode, or affect other performce-relevant
>> settings. Its also looks like you can steal the MAC address of a
>> different macvlan device this way and have the packets directed to you
>> (new devices are added to the beginning of the hash chains, so they
>> are found first on lookups).
> 
> To a large extent those are things that we already can do, simply by
> having multiple mcavlans in different network namespaces.  I could
> push it into promiscous mode by adding more multicast listeners,
> and I could steal the mac address of another macvlan by changing
> my mac address if I happen to come first in the hash chain.
> 
> Hmm.  Actually that appears to be a macvlan bug.  It looks like if I
> change the macaddress on a macvlan we don't update the hash chains.
> So unless we have the same low byte we will be on the wrong hash chain
> and not receive the packets for the mac we specified.  Ouch!

The address can only be changed while the device is down and unhashed.

> It is also trivial to spoof a different macvlan device by using
> PF_PACKET and sending packets with the source mac address of
> another macvlan.

Yes, but that doesn't allow one namespace to deny service to
a different one.

> Also this still requires CAP_NET_ADMIN, as much as I would like
> to remove that restriction.
> 
> Your concerns don't appear to be new to allowing the creation
> of a macvlan from a macvlan or fundamental to creating
> a macvlan from a macvlan.  You still must have access to at
> least a macvlan in your namespace to create a new one.  So
> I don't think those issues bear on my patch.

No, they're not, but it seemed worth pointing out. Your patch
looks perfectly fine.

Acked-by: Patrick McHardy <kaber@trash.net>

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[Eric W. Biederman]

Patrick McHardy <kaber@trash.net> writes:

> Eric W. Biederman wrote:
>> Patrick McHardy <kaber@trash.net> writes:
>>
>>> That makes sense of course. I'm mainly wondering whether a namespace
>>> should be able to directly affect the real device like this. This
>>> might move it to promiscous mode, or affect other performce-relevant
>>> settings. Its also looks like you can steal the MAC address of a
>>> different macvlan device this way and have the packets directed to you
>>> (new devices are added to the beginning of the hash chains, so they
>>> are found first on lookups).
>>
>> To a large extent those are things that we already can do, simply by
>> having multiple mcavlans in different network namespaces.  I could
>> push it into promiscous mode by adding more multicast listeners,
>> and I could steal the mac address of another macvlan by changing
>> my mac address if I happen to come first in the hash chain.
>>
>> Hmm.  Actually that appears to be a macvlan bug.  It looks like if I
>> change the macaddress on a macvlan we don't update the hash chains.
>> So unless we have the same low byte we will be on the wrong hash chain
>> and not receive the packets for the mac we specified.  Ouch!
>
> The address can only be changed while the device is down and unhashed.

Point.  The dev_unicast/dev_unicast_delete in macvlan_set_mac_address
appears to be completely unnecessary then.

>> It is also trivial to spoof a different macvlan device by using
>> PF_PACKET and sending packets with the source mac address of
>> another macvlan.
>
> Yes, but that doesn't allow one namespace to deny service to
> a different one.

No for that I guess it seems I just need to down the interface
change the mac and up the interface again.

>> Also this still requires CAP_NET_ADMIN, as much as I would like
>> to remove that restriction.
>>
>> Your concerns don't appear to be new to allowing the creation
>> of a macvlan from a macvlan or fundamental to creating
>> a macvlan from a macvlan.  You still must have access to at
>> least a macvlan in your namespace to create a new one.  So
>> I don't think those issues bear on my patch.
>
> No, they're not, but it seemed worth pointing out. Your patch
> looks perfectly fine.

Thanks.

Would you be opposed to changes that made macvlan more robust.
Such as refusing to come up if the macaddress is already in use.
And perhaps denying the sending of packets with the wrong source
mac?

> Acked-by: Patrick McHardy <kaber@trash.net>

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[Patrick McHardy]

Eric W. Biederman wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
>>> Hmm.  Actually that appears to be a macvlan bug.  It looks like if I
>>> change the macaddress on a macvlan we don't update the hash chains.
>>> So unless we have the same low byte we will be on the wrong hash chain
>>> and not receive the packets for the mac we specified.  Ouch!
>> The address can only be changed while the device is down and unhashed.
> 
> Point.  The dev_unicast/dev_unicast_delete in macvlan_set_mac_address
> appears to be completely unnecessary then.

I think thats correct.

>> No, they're not, but it seemed worth pointing out. Your patch
>> looks perfectly fine.
> 
> Thanks.
> 
> Would you be opposed to changes that made macvlan more robust.
> Such as refusing to come up if the macaddress is already in use.
> And perhaps denying the sending of packets with the wrong source
> mac?

Refusing duplicate MACs (on one underlying device) makes sense, the
results are undefined currently. About the filtering - I don't like
the idea too much given that we already have multiple possiblities
to do that.

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[Eric W. Biederman]

Patrick McHardy <kaber@trash.net> writes:

> Eric W. Biederman wrote:
>> Patrick McHardy <kaber@trash.net> writes:
>>
>>>> Hmm.  Actually that appears to be a macvlan bug.  It looks like if I
>>>> change the macaddress on a macvlan we don't update the hash chains.
>>>> So unless we have the same low byte we will be on the wrong hash chain
>>>> and not receive the packets for the mac we specified.  Ouch!
>>> The address can only be changed while the device is down and unhashed.
>>
>> Point.  The dev_unicast/dev_unicast_delete in macvlan_set_mac_address
>> appears to be completely unnecessary then.
>
> I think thats correct.

Actually it is really weird.  We can change the mac address while
the devices is running but the code is broken because it does
not update the hash table.

> Refusing duplicate MACs (on one underlying device) makes sense, the
> results are undefined currently.

Then I will do that just for consistency.

> About the filtering - I don't like
> the idea too much given that we already have multiple possiblities
> to do that.

Agreed, and even more I can think of some reasons why would not want
that.  The observation I have is that if really want separation you
need separate vlans to the switch.  As the similar hardware based
features also don't perform egress filtering.

Eric

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[Patrick McHardy]

Eric W. Biederman wrote:
> Patrick McHardy <kaber@trash.net> writes:
> 
>> Eric W. Biederman wrote:
>>> Patrick McHardy <kaber@trash.net> writes:
>>>
>>>>> Hmm.  Actually that appears to be a macvlan bug.  It looks like if I
>>>>> change the macaddress on a macvlan we don't update the hash chains.
>>>>> So unless we have the same low byte we will be on the wrong hash chain
>>>>> and not receive the packets for the mac we specified.  Ouch!
>>>> The address can only be changed while the device is down and unhashed.
>>> Point.  The dev_unicast/dev_unicast_delete in macvlan_set_mac_address
>>> appears to be completely unnecessary then.
>> I think thats correct.
> 
> Actually it is really weird.  We can change the mac address while
> the devices is running but the code is broken because it does
> not update the hash table.

Thats strange. I know the assumption was that this can't be done.
But I can't find anything preventing it, not even in older versions.

>> Refusing duplicate MACs (on one underlying device) makes sense, the
>> results are undefined currently.
> 
> Then I will do that just for consistency.

Thanks.

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[Ben Greear]

Eric W. Biederman wrote:
> Thanks.
> Would you be opposed to changes that made macvlan more robust.
> Such as refusing to come up if the macaddress is already in use.
> And perhaps denying the sending of packets with the wrong source
> mac?
>   
I wouldn't deny sending with wrong source mac..ethernet interfaces can 
do this,
and mac-vlan should look as much like ethernet is possible.

I'm all for making it harder to mis-configure things (like dup macs on 
single interface,
etc).

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

[PATCH] macvlan: Deterministic ingress packet delivery

[Eric W. Biederman]

>From 22d0a3e4ffa91f85c224d171ed7cc2d64a6dda00 Mon Sep 17 00:00:00 2001
From: Eric Biederman <ebiederm@xmission.com>
Date: Fri, 6 Mar 2009 09:16:30 -0800
Subject: 

Changing the mac address when a macvlan device is up will leave the
device on the wrong hash chain making it impossible to receive
packets.

There is no checking of the mac address set on the macvlan.  Allowing
a misconfiguration to grab packets from the the underlying device or
another macvlan.

To resolve these problems I update the hash table of macvlans when the
mac address of a macvlan changes, and when updating the hash table
I verify that the new mac address is usable.

The result is well defined and predictable if not perfect handling of
mac vlan mac addresses.

To keep the code clear I have created a set of hash table maintenance
in macvlan so I am not open coding the hash function and the logic
needed to update the hash table all over the place.

Signed-off-by: Eric Biederman <ebiederm@aristanetworks.com>
---
 drivers/net/macvlan.c |   73 ++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 60 insertions(+), 13 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index b5241fc..70d3ef4 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -60,6 +60,47 @@ static struct macvlan_dev *macvlan_hash_lookup(const struct macvlan_port *port,
 	return NULL;
 }
 
+static void macvlan_hash_add(struct macvlan_dev *vlan)
+{
+	struct macvlan_port *port = vlan->port;
+	const unsigned char *addr = vlan->dev->dev_addr;
+
+	hlist_add_head_rcu(&vlan->hlist, &port->vlan_hash[addr[5]]);
+}
+
+static void macvlan_hash_del(struct macvlan_dev *vlan)
+{
+	hlist_del_rcu(&vlan->hlist);
+	synchronize_rcu();
+}
+
+static void macvlan_hash_change_addr(struct macvlan_dev *vlan,
+					const unsigned char *addr)
+{
+	macvlan_hash_del(vlan);
+	/* Now that we are unhashed it is safe to change the device
+	 * address without confusing packet delivery.
+	 */
+	memcpy(vlan->dev->dev_addr, addr, ETH_ALEN);
+	macvlan_hash_add(vlan);
+}
+
+static int macvlan_addr_busy(const struct macvlan_port *port,
+				const unsigned char *addr)
+{
+	/* Test to see if the specified multicast address is
+	 * currently in use by the underlying device or
+	 * another macvlan.
+	 */
+	if (memcmp(port->dev->dev_addr, addr, ETH_ALEN) == 0)
+		return 1;
+
+	if (macvlan_hash_lookup(port, addr))
+		return 1;
+
+	return 0;
+}
+
 static void macvlan_broadcast(struct sk_buff *skb,
 			      const struct macvlan_port *port)
 {
@@ -184,10 +225,13 @@ static const struct header_ops macvlan_hard_header_ops = {
 static int macvlan_open(struct net_device *dev)
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
-	struct macvlan_port *port = vlan->port;
 	struct net_device *lowerdev = vlan->lowerdev;
 	int err;
 
+	err = -EBUSY;
+	if (macvlan_addr_busy(vlan->port, dev->dev_addr))
+		goto out;
+
 	err = dev_unicast_add(lowerdev, dev->dev_addr, ETH_ALEN);
 	if (err < 0)
 		goto out;
@@ -196,8 +240,7 @@ static int macvlan_open(struct net_device *dev)
 		if (err < 0)
 			goto del_unicast;
 	}
-
-	hlist_add_head_rcu(&vlan->hlist, &port->vlan_hash[dev->dev_addr[5]]);
+	macvlan_hash_add(vlan);
 	return 0;
 
 del_unicast:
@@ -217,8 +260,7 @@ static int macvlan_stop(struct net_device *dev)
 
 	dev_unicast_delete(lowerdev, dev->dev_addr, ETH_ALEN);
 
-	hlist_del_rcu(&vlan->hlist);
-	synchronize_rcu();
+	macvlan_hash_del(vlan);
 	return 0;
 }
 
@@ -232,16 +274,21 @@ static int macvlan_set_mac_address(struct net_device *dev, void *p)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
-	if (!(dev->flags & IFF_UP))
-		goto out;
+	if (!(dev->flags & IFF_UP)) {
+		/* Just copy in the new address */
+		memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
+	} else {
+		/* Rehash and update the device filters */
+		if (macvlan_addr_busy(vlan->port, addr->sa_data))
+			return -EBUSY;
 
-	err = dev_unicast_add(lowerdev, addr->sa_data, ETH_ALEN);
-	if (err < 0)
-		return err;
-	dev_unicast_delete(lowerdev, dev->dev_addr, ETH_ALEN);
+		if ((err = dev_unicast_add(lowerdev, addr->sa_data, ETH_ALEN)))
+			return err;
 
-out:
-	memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
+		dev_unicast_delete(lowerdev, dev->dev_addr, ETH_ALEN);
+
+		macvlan_hash_change_addr(vlan, addr->sa_data);
+	}
 	return 0;
 }
 
-- 
1.6.1.2.350.g88cc

Re: [PATCH] macvlan: Deterministic ingress packet delivery

[Ben Greear]

Eric W. Biederman wrote:
> >From 22d0a3e4ffa91f85c224d171ed7cc2d64a6dda00 Mon Sep 17 00:00:00 2001
> From: Eric Biederman <ebiederm@xmission.com>
> Date: Fri, 6 Mar 2009 09:16:30 -0800
> Subject: 
>
> Changing the mac address when a macvlan device is up will leave the
> device on the wrong hash chain making it impossible to receive
> packets.
>   
I see no problem with this, but Patrick knows the code better...

Might be nice to use a bigger hash some day, but can deal with that 
separately.

Ben

-- 
Ben Greear <greearb@candelatech.com> 
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] macvlan: Deterministic ingress packet delivery

[Patrick McHardy]

Eric W. Biederman wrote:
>>From 22d0a3e4ffa91f85c224d171ed7cc2d64a6dda00 Mon Sep 17 00:00:00 2001
> From: Eric Biederman <ebiederm@xmission.com>
> Date: Fri, 6 Mar 2009 09:16:30 -0800
> Subject: 
> 
> Changing the mac address when a macvlan device is up will leave the
> device on the wrong hash chain making it impossible to receive
> packets.
> 
> There is no checking of the mac address set on the macvlan.  Allowing
> a misconfiguration to grab packets from the the underlying device or
> another macvlan.
> 
> To resolve these problems I update the hash table of macvlans when the
> mac address of a macvlan changes, and when updating the hash table
> I verify that the new mac address is usable.
> 
> The result is well defined and predictable if not perfect handling of
> mac vlan mac addresses.
> 
> To keep the code clear I have created a set of hash table maintenance
> in macvlan so I am not open coding the hash function and the logic
> needed to update the hash table all over the place.

This looks fine, thanks.

Acked-by: Patrick McHardy <kaber@trash.net>

Re: [PATCH] macvlan: Deterministic ingress packet delivery

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon, 09 Mar 2009 14:25:11 +0100

> Eric W. Biederman wrote:
> >>From 22d0a3e4ffa91f85c224d171ed7cc2d64a6dda00 Mon Sep 17 00:00:00 2001
> > From: Eric Biederman <ebiederm@xmission.com>
> > Date: Fri, 6 Mar 2009 09:16:30 -0800
> > Subject: Changing the mac address when a macvlan device is up will leave the
> > device on the wrong hash chain making it impossible to receive
> > packets.
> > There is no checking of the mac address set on the macvlan.  Allowing
> > a misconfiguration to grab packets from the the underlying device or
> > another macvlan.
> > To resolve these problems I update the hash table of macvlans when the
> > mac address of a macvlan changes, and when updating the hash table
> > I verify that the new mac address is usable.
> > The result is well defined and predictable if not perfect handling of
> > mac vlan mac addresses.
> > To keep the code clear I have created a set of hash table maintenance
> > in macvlan so I am not open coding the hash function and the logic
> > needed to update the hash table all over the place.
> 
> This looks fine, thanks.
> 
> Acked-by: Patrick McHardy <kaber@trash.net>

Also applied, thanks.

Re: [PATCH] macvlan: Support creating macvlans from macvlans

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 06 Mar 2009 16:08:45 +0100

> Acked-by: Patrick McHardy <kaber@trash.net>

Applied to net-next-2.6