[PATCH 1/2] veth: Initialize dev->perm_addr

[PATCH 1/2] veth: Initialize dev->perm_addr

[Mira Ressel]

Set the perm_addr of veth devices to whatever MAC has been assigned to
the device. Otherwise, it remains all zero, with the consequence that
ipv6_generate_stable_address() (which is used if the sysctl
net.ipv6.conf.DEV.addr_gen_mode is set to 2 or 3) assigns every veth
interface on a host the same link-local address.

The new behaviour matches that of several other virtual interface types
(such as gre), and as far as I can tell, perm_addr isn't used by any
other code sites that are relevant to veth.

Signed-off-by: Mira Ressel <aranea@aixah.de>
---
 drivers/net/veth.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index e56cd562a664..af1a7cda6205 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1342,6 +1342,8 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	if (!ifmp || !tbp[IFLA_ADDRESS])
 		eth_hw_addr_random(peer);
 
+	memcpy(peer->perm_addr, peer->dev_addr, peer->addr_len);
+
 	if (ifmp && (dev->ifindex != 0))
 		peer->ifindex = ifmp->ifi_index;
 
@@ -1370,6 +1372,8 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
 	if (tb[IFLA_ADDRESS] == NULL)
 		eth_hw_addr_random(dev);
 
+	memcpy(dev->perm_addr, dev->dev_addr, dev->addr_len);
+
 	if (tb[IFLA_IFNAME])
 		nla_strlcpy(dev->name, tb[IFLA_IFNAME], IFNAMSIZ);
 	else
-- 
2.25.4

[PATCH 2/2] vlan: Initialize dev->perm_addr

[Mira Ressel]

Set the perm_addr of vlan devices to that of their parent device.
Otherwise, it remains all zero, with the consequence that
ipv6_generate_stable_address() (which is used if the sysctl
net.ipv6.conf.DEV.addr_gen_mode is set to 2 or 3) assigns every vlan
interface on a host the same link-local address.

This has the added benefit of giving vlan devices the same link-local
address as their parent device, which is common practice, and indeed
precisely what happens automatically if the default eui64-based address
generation is used.

Signed-off-by: Mira Ressel <aranea@aixah.de>
---
 net/8021q/vlan_netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
index 0db85aeb119b..8c60d92b7717 100644
--- a/net/8021q/vlan_netlink.c
+++ b/net/8021q/vlan_netlink.c
@@ -182,6 +182,8 @@ static int vlan_newlink(struct net *src_net, struct net_device *dev,
 	else if (dev->mtu > max_mtu)
 		return -EINVAL;
 
+	memcpy(dev->perm_addr, real_dev->perm_addr, real_dev->addr_len);
+
 	err = vlan_changelink(dev, tb, data, extack);
 	if (!err)
 		err = register_vlan_dev(dev, extack);
-- 
2.25.4

Re: [PATCH 1/2] veth: Initialize dev->perm_addr

[David Miller]

From: Mira Ressel <aranea@aixah.de>
Date: Mon, 24 Aug 2020 14:38:26 +0000

> Set the perm_addr of veth devices to whatever MAC has been assigned to
> the device. Otherwise, it remains all zero, with the consequence that
> ipv6_generate_stable_address() (which is used if the sysctl
> net.ipv6.conf.DEV.addr_gen_mode is set to 2 or 3) assigns every veth
> interface on a host the same link-local address.
> 
> The new behaviour matches that of several other virtual interface types
> (such as gre), and as far as I can tell, perm_addr isn't used by any
> other code sites that are relevant to veth.
> 
> Signed-off-by: Mira Ressel <aranea@aixah.de>
 ...
> @@ -1342,6 +1342,8 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
>  	if (!ifmp || !tbp[IFLA_ADDRESS])
>  		eth_hw_addr_random(peer);
>  
> +	memcpy(peer->perm_addr, peer->dev_addr, peer->addr_len);

Semantically don't you want to copy over the peer->perm_addr?

Otherwise this loses the entire point of the permanent address, and
what the ipv6 address generation facility expects.

Re: [PATCH 1/2] veth: Initialize dev->perm_addr

[Mira Ressel]

On Mon, Aug 24, 2020 at 10:25:45AM -0700, David Miller wrote:
> From: Mira Ressel <aranea@aixah.de>
> Date: Mon, 24 Aug 2020 14:38:26 +0000
> 
> > Set the perm_addr of veth devices to whatever MAC has been assigned to
> > the device. Otherwise, it remains all zero, with the consequence that
> > ipv6_generate_stable_address() (which is used if the sysctl
> > net.ipv6.conf.DEV.addr_gen_mode is set to 2 or 3) assigns every veth
> > interface on a host the same link-local address.
> > 
> > The new behaviour matches that of several other virtual interface types
> > (such as gre), and as far as I can tell, perm_addr isn't used by any
> > other code sites that are relevant to veth.
> > 
> > Signed-off-by: Mira Ressel <aranea@aixah.de>
>  ...
> > @@ -1342,6 +1342,8 @@ static int veth_newlink(struct net *src_net, struct net_device *dev,
> >  	if (!ifmp || !tbp[IFLA_ADDRESS])
> >  		eth_hw_addr_random(peer);
> >  
> > +	memcpy(peer->perm_addr, peer->dev_addr, peer->addr_len);
> 
> Semantically don't you want to copy over the peer->perm_addr?
> 
> Otherwise this loses the entire point of the permanent address, and
> what the ipv6 address generation facility expects.

I'm confused. Am I misinterpreting what you're saying here, or did you
make a typo?

I'm setting the peer->perm_addr, which would otherwise be zero, to its
dev_addr, which has been either generated randomly by the kernel or
provided by userland in a netlink attribute.

-- 
Regards,
Mira

Re: [PATCH 1/2] veth: Initialize dev->perm_addr

[David Miller]

From: Mira Ressel <aranea@aixah.de>
Date: Wed, 26 Aug 2020 15:20:00 +0000

> I'm setting the peer->perm_addr, which would otherwise be zero, to its
> dev_addr, which has been either generated randomly by the kernel or
> provided by userland in a netlink attribute.

Which by definition makes it not necessarily a "permanent address" and
therefore is subject to being different across boots, which is exactly
what you don't want to happen for automatic address generation.

Re: [PATCH 1/2] veth: Initialize dev->perm_addr

[Mira Ressel]

On Wed, Aug 26, 2020 at 08:28:57AM -0700, David Miller wrote:
> From: Mira Ressel <aranea@aixah.de>
> Date: Wed, 26 Aug 2020 15:20:00 +0000
> 
> > I'm setting the peer->perm_addr, which would otherwise be zero, to its
> > dev_addr, which has been either generated randomly by the kernel or
> > provided by userland in a netlink attribute.
> 
> Which by definition makes it not necessarily a "permanent address" and
> therefore is subject to being different across boots, which is exactly
> what you don't want to happen for automatic address generation.

That's true, but since veth devices aren't backed by any hardware, I
unfortunately don't have a good source for a permanent address. The only
inherently permanent thing about them is their name.

People who use the default eui64-based address generation don't get
persistent link-local addresses for their veth devices out of the box
either -- the EUI64 is derived from the device's dev_addr, which is
randomized by default.

If that presents a problem for anyone, they can configure their userland
to set the dev_addr to a static value, which handily fixes this problem
for both address generation algorithms.

I'm admittedly glancing over one problem here -- I'm only setting the
perm_addr during device creation, whereas userland can change the
dev_addr at any time. I'm not sure if it'd make sense here to update the
perm_addr if the dev_addr is changed later on?

-- 
Regards,
Mira

Re: [PATCH 1/2] veth: Initialize dev->perm_addr

[David Miller]

From: Mira Ressel <aranea@aixah.de>
Date: Wed, 26 Aug 2020 16:29:01 +0000

> On Wed, Aug 26, 2020 at 08:28:57AM -0700, David Miller wrote:
>> From: Mira Ressel <aranea@aixah.de>
>> Date: Wed, 26 Aug 2020 15:20:00 +0000
>> 
>> > I'm setting the peer->perm_addr, which would otherwise be zero, to its
>> > dev_addr, which has been either generated randomly by the kernel or
>> > provided by userland in a netlink attribute.
>> 
>> Which by definition makes it not necessarily a "permanent address" and
>> therefore is subject to being different across boots, which is exactly
>> what you don't want to happen for automatic address generation.
> 
> That's true, but since veth devices aren't backed by any hardware, I
> unfortunately don't have a good source for a permanent address. The only
> inherently permanent thing about them is their name.
> 
> People who use the default eui64-based address generation don't get
> persistent link-local addresses for their veth devices out of the box
> either -- the EUI64 is derived from the device's dev_addr, which is
> randomized by default.
> 
> If that presents a problem for anyone, they can configure their userland
> to set the dev_addr to a static value, which handily fixes this problem
> for both address generation algorithms.
> 
> I'm admittedly glancing over one problem here -- I'm only setting the
> perm_addr during device creation, whereas userland can change the
> dev_addr at any time. I'm not sure if it'd make sense here to update the
> perm_addr if the dev_addr is changed later on?

We are talking about which parent device address to inherit from, you
have choosen to use dev_addr and I am saying you should use perm_addr.

Can you explain why this isn't clear?

Re: [PATCH 1/2] veth: Initialize dev->perm_addr

[Mira Ressel]

On Wed, Aug 26, 2020 at 09:33:29AM -0700, David Miller wrote:
> From: Mira Ressel <aranea@aixah.de>
> Date: Wed, 26 Aug 2020 16:29:01 +0000
> 
> > On Wed, Aug 26, 2020 at 08:28:57AM -0700, David Miller wrote:
> >> From: Mira Ressel <aranea@aixah.de>
> >> Date: Wed, 26 Aug 2020 15:20:00 +0000
> >> 
> >> > I'm setting the peer->perm_addr, which would otherwise be zero, to its
> >> > dev_addr, which has been either generated randomly by the kernel or
> >> > provided by userland in a netlink attribute.
> >> 
> >> Which by definition makes it not necessarily a "permanent address" and
> >> therefore is subject to being different across boots, which is exactly
> >> what you don't want to happen for automatic address generation.
> > 
> > That's true, but since veth devices aren't backed by any hardware, I
> > unfortunately don't have a good source for a permanent address. The only
> > inherently permanent thing about them is their name.
> > 
> > People who use the default eui64-based address generation don't get
> > persistent link-local addresses for their veth devices out of the box
> > either -- the EUI64 is derived from the device's dev_addr, which is
> > randomized by default.
> > 
> > If that presents a problem for anyone, they can configure their userland
> > to set the dev_addr to a static value, which handily fixes this problem
> > for both address generation algorithms.
> > 
> > I'm admittedly glancing over one problem here -- I'm only setting the
> > perm_addr during device creation, whereas userland can change the
> > dev_addr at any time. I'm not sure if it'd make sense here to update the
> > perm_addr if the dev_addr is changed later on?
> 
> We are talking about which parent device address to inherit from, you
> have choosen to use dev_addr and I am saying you should use perm_addr.
> 
> Can you explain why this isn't clear?

Which parent device? This is the veth patch, not the vlan patch. I'm
setting the perm_addrs of both sides of the veth pair to their
respective dev_addrs that they were assigned by userland or through
random generation. If I were to give both of them the same perm_addr,
we'd again have the problem that they'd get conflicting link-local
addresses and trigger DAD.

My apologies if I'm misunderstanding something here!

Regards,
Mira