* [PATCH net v4] vlan: Propagate MAC address to VLANs [not found] <572DB364.2040109@brocade.com> @ 2016-05-07 10:00 ` Mike Manning 2016-05-09 6:48 ` Michal Kubecek 0 siblings, 1 reply; 3+ messages in thread From: Mike Manning @ 2016-05-07 10:00 UTC (permalink / raw) To: netdev The MAC address of the physical interface is only copied to the VLAN when it is first created, resulting in an inconsistency after MAC address changes of only newly created VLANs having an up-to-date MAC. The VLANs should continue inheriting the MAC address of the physical interface until the VLAN MAC address is explicitly set to any value. This allows IPv6 EUI64 addresses for the VLAN to reflect any changes to the MAC of the physical interface and thus for DAD to behave as expected. Signed-off-by: Mike Manning <mmanning@brocade.com> --- net/8021q/vlan.c | 7 +++++++ net/8021q/vlan_dev.c | 14 ++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) --- a/net/8021q/vlan.c +++ b/net/8021q/vlan.c @@ -291,6 +291,12 @@ static void vlan_sync_address(struct net if (ether_addr_equal(vlan->real_dev_addr, dev->dev_addr)) return; + /* vlan continues to inherit address of parent interface */ + if (vlandev->addr_assign_type == NET_ADDR_STOLEN) { + ether_addr_copy(vlandev->dev_addr, dev->dev_addr); + goto out; + } + /* vlan address was different from the old address and is equal to * the new address */ if (!ether_addr_equal(vlandev->dev_addr, vlan->real_dev_addr) && @@ -303,6 +309,7 @@ static void vlan_sync_address(struct net !ether_addr_equal(vlandev->dev_addr, dev->dev_addr)) dev_uc_add(dev, vlandev->dev_addr); +out: ether_addr_copy(vlan->real_dev_addr, dev->dev_addr); } --- a/net/8021q/vlan_dev.c +++ b/net/8021q/vlan_dev.c @@ -255,9 +255,13 @@ static int vlan_dev_open(struct net_devi return -ENETDOWN; if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr)) { - err = dev_uc_add(real_dev, dev->dev_addr); - if (err < 0) - goto out; + if (dev->addr_assign_type == NET_ADDR_STOLEN) { + ether_addr_copy(dev->dev_addr, real_dev->dev_addr); + } else { + err = dev_uc_add(real_dev, dev->dev_addr); + if (err < 0) + goto out; + } } if (dev->flags & IFF_ALLMULTI) { @@ -558,8 +562,10 @@ static int vlan_dev_init(struct net_devi /* ipv6 shared card related stuff */ dev->dev_id = real_dev->dev_id; - if (is_zero_ether_addr(dev->dev_addr)) + if (is_zero_ether_addr(dev->dev_addr)) { eth_hw_addr_inherit(dev, real_dev); + dev->addr_assign_type = NET_ADDR_STOLEN; + } if (is_zero_ether_addr(dev->broadcast)) memcpy(dev->broadcast, real_dev->broadcast, dev->addr_len); -- 1.7.10.4 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net v4] vlan: Propagate MAC address to VLANs 2016-05-07 10:00 ` [PATCH net v4] vlan: Propagate MAC address to VLANs Mike Manning @ 2016-05-09 6:48 ` Michal Kubecek 2016-05-09 12:19 ` Mike Manning 0 siblings, 1 reply; 3+ messages in thread From: Michal Kubecek @ 2016-05-09 6:48 UTC (permalink / raw) To: Mike Manning; +Cc: netdev On Sat, May 07, 2016 at 11:00:09AM +0100, Mike Manning wrote: > The MAC address of the physical interface is only copied to the VLAN > when it is first created, resulting in an inconsistency after MAC > address changes of only newly created VLANs having an up-to-date MAC. > > The VLANs should continue inheriting the MAC address of the physical > interface until the VLAN MAC address is explicitly set to any value. > This allows IPv6 EUI64 addresses for the VLAN to reflect any changes > to the MAC of the physical interface and thus for DAD to behave as > expected. > > Signed-off-by: Mike Manning <mmanning@brocade.com> > --- > net/8021q/vlan.c | 7 +++++++ > net/8021q/vlan_dev.c | 14 ++++++++++---- > 2 files changed, 17 insertions(+), 4 deletions(-) > > --- a/net/8021q/vlan.c > +++ b/net/8021q/vlan.c > @@ -291,6 +291,12 @@ static void vlan_sync_address(struct net > if (ether_addr_equal(vlan->real_dev_addr, dev->dev_addr)) > return; > > + /* vlan continues to inherit address of parent interface */ > + if (vlandev->addr_assign_type == NET_ADDR_STOLEN) { > + ether_addr_copy(vlandev->dev_addr, dev->dev_addr); > + goto out; > + } > + I might have missed something in the previous discussion but as ether_addr_copy() is just an optimized memcpy(), how is this going to handle the setups where the vlan device itself has an upper device? For example, - if it is a bridge port, how is the bridge going to learn about its address change so that it can update its FDB? - if it is a bond slave or team port, current code preserves the vlan device address on real device change so everything is fine; your proposal would change vlan device's address without bond being even notified, I believe - there might be a macvlan on top of the vlan and you might accidentally match its address with the new one > /* vlan address was different from the old address and is equal to > * the new address */ > if (!ether_addr_equal(vlandev->dev_addr, vlan->real_dev_addr) && > @@ -303,6 +309,7 @@ static void vlan_sync_address(struct net > !ether_addr_equal(vlandev->dev_addr, dev->dev_addr)) > dev_uc_add(dev, vlandev->dev_addr); > > +out: > ether_addr_copy(vlan->real_dev_addr, dev->dev_addr); > } > > --- a/net/8021q/vlan_dev.c > +++ b/net/8021q/vlan_dev.c > @@ -255,9 +255,13 @@ static int vlan_dev_open(struct net_devi > return -ENETDOWN; > > if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr)) { > - err = dev_uc_add(real_dev, dev->dev_addr); > - if (err < 0) > - goto out; > + if (dev->addr_assign_type == NET_ADDR_STOLEN) { > + ether_addr_copy(dev->dev_addr, real_dev->dev_addr); The same question here. > + } else { > + err = dev_uc_add(real_dev, dev->dev_addr); > + if (err < 0) > + goto out; > + } > } > > if (dev->flags & IFF_ALLMULTI) { > @@ -558,8 +562,10 @@ static int vlan_dev_init(struct net_devi > /* ipv6 shared card related stuff */ > dev->dev_id = real_dev->dev_id; > > - if (is_zero_ether_addr(dev->dev_addr)) > + if (is_zero_ether_addr(dev->dev_addr)) { > eth_hw_addr_inherit(dev, real_dev); > + dev->addr_assign_type = NET_ADDR_STOLEN; You might want to replace eth_hw_addr_inherit() with ether_addr_copy() here as they only differ in the former copying addr_assign_type which you are going to rewrite anyway. (But as both are most likely inlined, I would expect the resulting code to be the same in the end.) Michal Kubecek > + } > if (is_zero_ether_addr(dev->broadcast)) > memcpy(dev->broadcast, real_dev->broadcast, dev->addr_len); > > -- > 1.7.10.4 > > > > > > > > > > > > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net v4] vlan: Propagate MAC address to VLANs 2016-05-09 6:48 ` Michal Kubecek @ 2016-05-09 12:19 ` Mike Manning 0 siblings, 0 replies; 3+ messages in thread From: Mike Manning @ 2016-05-09 12:19 UTC (permalink / raw) To: Michal Kubecek; +Cc: netdev On 05/09/2016 07:48 AM, Michal Kubecek wrote: > On Sat, May 07, 2016 at 11:00:09AM +0100, Mike Manning wrote: >> The MAC address of the physical interface is only copied to the VLAN >> when it is first created, resulting in an inconsistency after MAC >> address changes of only newly created VLANs having an up-to-date MAC. >> >> The VLANs should continue inheriting the MAC address of the physical >> interface until the VLAN MAC address is explicitly set to any value. >> This allows IPv6 EUI64 addresses for the VLAN to reflect any changes >> to the MAC of the physical interface and thus for DAD to behave as >> expected. >> >> Signed-off-by: Mike Manning <mmanning@brocade.com> >> --- >> net/8021q/vlan.c | 7 +++++++ >> net/8021q/vlan_dev.c | 14 ++++++++++---- >> 2 files changed, 17 insertions(+), 4 deletions(-) >> >> --- a/net/8021q/vlan.c >> +++ b/net/8021q/vlan.c >> @@ -291,6 +291,12 @@ static void vlan_sync_address(struct net >> if (ether_addr_equal(vlan->real_dev_addr, dev->dev_addr)) >> return; >> >> + /* vlan continues to inherit address of parent interface */ >> + if (vlandev->addr_assign_type == NET_ADDR_STOLEN) { >> + ether_addr_copy(vlandev->dev_addr, dev->dev_addr); >> + goto out; >> + } >> + > > I might have missed something in the previous discussion but as > ether_addr_copy() is just an optimized memcpy(), how is this going to > handle the setups where the vlan device itself has an upper device? For > example, > > - if it is a bridge port, how is the bridge going to learn about its > address change so that it can update its FDB? > - if it is a bond slave or team port, current code preserves the vlan > device address on real device change so everything is fine; your > proposal would change vlan device's address without bond being even > notified, I believe > - there might be a macvlan on top of the vlan and you might > accidentally match its address with the new one > Thanks for your review and this excellent catch. I will add address notification for the vlan itself and test appropriately for when an upper device is present. >> /* vlan address was different from the old address and is equal to >> * the new address */ >> if (!ether_addr_equal(vlandev->dev_addr, vlan->real_dev_addr) && >> @@ -303,6 +309,7 @@ static void vlan_sync_address(struct net >> !ether_addr_equal(vlandev->dev_addr, dev->dev_addr)) >> dev_uc_add(dev, vlandev->dev_addr); >> >> +out: >> ether_addr_copy(vlan->real_dev_addr, dev->dev_addr); >> } >> >> --- a/net/8021q/vlan_dev.c >> +++ b/net/8021q/vlan_dev.c >> @@ -255,9 +255,13 @@ static int vlan_dev_open(struct net_devi >> return -ENETDOWN; >> >> if (!ether_addr_equal(dev->dev_addr, real_dev->dev_addr)) { >> - err = dev_uc_add(real_dev, dev->dev_addr); >> - if (err < 0) >> - goto out; >> + if (dev->addr_assign_type == NET_ADDR_STOLEN) { >> + ether_addr_copy(dev->dev_addr, real_dev->dev_addr); > > The same question here. > >> + } else { >> + err = dev_uc_add(real_dev, dev->dev_addr); >> + if (err < 0) >> + goto out; >> + } >> } >> >> if (dev->flags & IFF_ALLMULTI) { >> @@ -558,8 +562,10 @@ static int vlan_dev_init(struct net_devi >> /* ipv6 shared card related stuff */ >> dev->dev_id = real_dev->dev_id; >> >> - if (is_zero_ether_addr(dev->dev_addr)) >> + if (is_zero_ether_addr(dev->dev_addr)) { >> eth_hw_addr_inherit(dev, real_dev); >> + dev->addr_assign_type = NET_ADDR_STOLEN; > > You might want to replace eth_hw_addr_inherit() with ether_addr_copy() > here as they only differ in the former copying addr_assign_type which > you are going to rewrite anyway. (But as both are most likely inlined, > I would expect the resulting code to be the same in the end.) > > Michal Kubecek > Thanks. Yes, I was aware of this but decided not to change it to keep the changeset to a minimum. I will make the change as recommended. >> + } >> if (is_zero_ether_addr(dev->broadcast)) >> memcpy(dev->broadcast, real_dev->broadcast, dev->addr_len); >> >> -- >> 1.7.10.4 >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-05-09 12:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <572DB364.2040109@brocade.com>
2016-05-07 10:00 ` [PATCH net v4] vlan: Propagate MAC address to VLANs Mike Manning
2016-05-09 6:48 ` Michal Kubecek
2016-05-09 12:19 ` Mike Manning
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.