| Message ID | 20200824143828.5964-1-aranea@aixah.de |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/2] veth: Initialize dev->perm_addr | expand |
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.
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.
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.
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?
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?
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
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
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(+)