[PATCH net] vlan: Propagate MAC address changes properly

[PATCH net] vlan: Propagate MAC address changes properly

[Mike Manning]

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.

Continuing to inherit the MAC address unless explicitly changed for
the VLAN allows IPv6 EUI64 addresses for the VLAN to reflect the change
and thus for DAD to behave as expected for the given MAC.

Signed-off-by: Mike Manning <mmanning@brocade.com>
---
 net/8021q/vlan.c |   17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index d2cd9de..2f57cf2 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -293,15 +293,15 @@ static void vlan_sync_address(struct net_device *dev,
 
 	/* 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) &&
+	if ((vlandev->flags & IFF_UP) &&
+	    !ether_addr_equal(vlandev->dev_addr, vlan->real_dev_addr) &&
 	    ether_addr_equal(vlandev->dev_addr, dev->dev_addr))
 		dev_uc_del(dev, vlandev->dev_addr);
 
-	/* vlan address was equal to the old address and is different from
+	/* vlan address was equal to the old address so now also inherit
 	 * the new address */
-	if (ether_addr_equal(vlandev->dev_addr, vlan->real_dev_addr) &&
-	    !ether_addr_equal(vlandev->dev_addr, dev->dev_addr))
-		dev_uc_add(dev, vlandev->dev_addr);
+	if (ether_addr_equal(vlandev->dev_addr, vlan->real_dev_addr))
+		ether_addr_copy(vlandev->dev_addr, dev->dev_addr);
 
 	ether_addr_copy(vlan->real_dev_addr, dev->dev_addr);
 }
@@ -389,13 +389,8 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 
 	case NETDEV_CHANGEADDR:
 		/* Adjust unicast filters on underlying device */
-		vlan_group_for_each_dev(grp, i, vlandev) {
-			flgs = vlandev->flags;
-			if (!(flgs & IFF_UP))
-				continue;
-
+		vlan_group_for_each_dev(grp, i, vlandev)
 			vlan_sync_address(dev, vlandev);
-		}
 		break;
 
 	case NETDEV_CHANGEMTU:
-- 
1.7.10.4

Re: [PATCH net] vlan: Propagate MAC address changes properly

[David Miller]

From: Mike Manning <mmanning@brocade.com>
Date: Sat, 30 Apr 2016 11:32:37 +0100

> 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.
> 
> Continuing to inherit the MAC address unless explicitly changed for
> the VLAN allows IPv6 EUI64 addresses for the VLAN to reflect the change
> and thus for DAD to behave as expected for the given MAC.
> 
> Signed-off-by: Mike Manning <mmanning@brocade.com>

What is this code really trying to achieve?

Is it "Propagate real device MAC changes to undelying vlan device,
but not if the user set the vlan MAC explicitly."?

If so, implement that instead of all of these confusing tests.

If the vlan device's set_mac_address operation is ever called,
set a boolean value in the vlan device private to true and test
it here.

Re: [PATCH net] vlan: Propagate MAC address changes properly

[Mike Manning]

On 05/03/2016 05:16 AM, David Miller wrote:
> From: Mike Manning <mmanning@brocade.com>
> Date: Sat, 30 Apr 2016 11:32:37 +0100
> 
>> 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.
>>
>> Continuing to inherit the MAC address unless explicitly changed for
>> the VLAN allows IPv6 EUI64 addresses for the VLAN to reflect the change
>> and thus for DAD to behave as expected for the given MAC.
>>
>> Signed-off-by: Mike Manning <mmanning@brocade.com>
> 
> What is this code really trying to achieve?
> 
> Is it "Propagate real device MAC changes to undelying vlan device,
> but not if the user set the vlan MAC explicitly."?

Right, I will update the subject header to make this clearer

> 
> If so, implement that instead of all of these confusing tests.
> 
> If the vlan device's set_mac_address operation is ever called,
> set a boolean value in the vlan device private to true and test
> it here.
> 

Given that this information is implicit in real_dev_addr, I am reluctant
to add another member to the vlan_dev_priv data structure, especially given
that there may be a large number of VLANs. Instead I have added a variable
real_addr_in_use in vlan_sync_address() to make this clearer.

Re: [PATCH net] vlan: Propagate MAC address changes properly

[David Miller]

From: Mike Manning <mmanning@brocade.com>
Date: Tue, 3 May 2016 16:18:42 +0100

> Given that this information is implicit in real_dev_addr

That's not the reason to make or not make the simplification I
asked you do.

You can add a multi-page comment to this code, and the confusing
tests are still rediculous.

Please add the boolean state as I asked you to.