Propagate MAC address changes to VLANs

PATCH In review Mike Manning 2 participants 3 messages Feb 29, 2016 – Apr 30, 2016

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Mike Manning

author

2016-02-29 11:32 UTC #0 lore raw

PATCH 1/1 Propagate MAC address changes to VLANs

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 <[email protected]>

---
 net/8021q/vlan.c |   17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

net/8021q/vlan.c +6 ~~-11~~

index d2cd9de..2f57cf2 100644
@@ -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:

Signature · 1 line

1.7.10.4

David Miller

maintainer

2016-03-03 21:12 UTC #1 lore raw

From: Mike Manning <[email protected]>
Date: Mon, 29 Feb 2016 11:32:51 +0000

net/8021q/vlan.c static void vlan_sync_address(struct net_device *dev, · line 304

Quoted 10 lines

net/8021q/vlan.c +3 -4

-	/* 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);

This dev_uc_add() call removal cannot be correct, if the device is up we must programe it into the hardware unicast filters and if also potentially put it into promiscuous mode via __dev_set_rx_mode().

Also your subject line isn't formatted properly, it should be:

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

Mike Manning

author

2016-04-30 10:29 UTC #2 lore raw

Quoted 4 lines · David Miller

From: Mike Manning <[email protected]>
Date: Mon, 29 Feb 2016 11:32:51 +0000
Quoted 1 line

Quoted 14 lines

net/8021q/vlan.c static void vlan_sync_address(struct net_device *dev, · line 304
Quoted 9 lines
net/8021q/vlan.c +3 -4
-	/* 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);
This dev_uc_add() call removal cannot be correct, if the device is up we must programe it into the hardware unicast filters and if also potentially put it into promiscuous mode via __dev_set_rx_mode().

The call to dev_uc_add() to add a secondary address is only needed if the VLAN MAC is different from that for the physical interface. For the proposed changes, the VLAN MAC is tracking that of the physical interface and so is the same (as typically it does not make sense for these to be different), so dev_uc_add() should not be called. The easiest way to demonstrate equivalence with the original code, where the MAC address has to be set manually, is with some test debugs. Here, first the MAC of the interface itself is changed (so dev_uc_add() is called), then the MAC of the VLAN is changed (so dev_uc_del() is called):

1) ORIGINAL CODE:

ip addr show dev dp0s8 | grep ether

    link/ether 52:54:00:1f:06:2a brd ff:ff:ff:ff:ff:ff
ip addr show dev dp0s8.40 | grep ether
    link/ether 52:54:00:1f:06:2a brd ff:ff:ff:ff:ff:ff
sudo ip link set dp0s8.40 addr 10:20:30:40:50:61
sudo ip link set dp0s8 addr 10:20:30:40:50:61
ip addr show dev dp0s8 | grep ether
    link/ether 10:20:30:40:50:61 brd ff:ff:ff:ff:ff:ff
ip addr show dev dp0s8.40 | grep ether
    link/ether 10:20:30:40:50:61 brd ff:ff:ff:ff:ff:ff

[ 3990.332577] --- vlan_dev_set_mac_address: id 40, call dev_uc_add for 10:20:30:40:50:61 on dp
0s8
[ 3990.332579] device dp0s8 entered promiscuous mode
[ 4002.425234] 8021q: --- vlan_sync_address: id 40, for 10:20:30:40:50:61 on dp0s8.40 (from dp0
s8)
[ 4002.425472] --- vlan_sync_address: id 40, call dev_uc_del for 10:20:30:40:50:61 on dp0s8
[ 4002.425475] --- __hw_addr_del_entry: refcount 0 for 10:20:30:40:50:61
[ 4002.425477] device dp0s8 left promiscuous mode

sudo ip link set dp0s8 addr 52:54:00:1f:06:2a
ip addr show dev dp0s8.40 | grep ether

    link/ether 10:20:30:40:50:61 brd ff:ff:ff:ff:ff:ff
sudo ip link set dp0s8.40 addr 52:54:00:1f:06:2a
ip addr show dev dp0s8 | grep ether
    link/ether 52:54:00:1f:06:2a brd ff:ff:ff:ff:ff:ff
ip addr show dev dp0s8.40 | grep ether
    link/ether 52:54:00:1f:06:2a brd ff:ff:ff:ff:ff:ff

[ 4121.606671] --- vlan_sync_address: id, 40, call dev_uc_add for 10:20:30:40:50:61 on dp0s8
[ 4121.606673] device dp0s8 entered promiscuous mode
[ 4147.487780] --- vlan_dev_set_mac_address: id 40, for 52:54:00:1f:06:2a on dp0s8
[ 4147.487782] --- vlan_dev_set_mac_address: id 40, call dev_uc_del for 10:20:30:40:50:61 dp0s8
[ 4147.487784] --- __hw_addr_del_entry: refcount 0 for 10:20:30:40:50:61
[ 4147.487786] device dp0s8 left promiscuous mode

2) WITH IMPROVEMENT FOR VLAN MAC TO FOLLOW THAT OF PHYSICAL INTF, UNLESS EXPLICITLY SET:

ip addr show dev dp0s8 | grep ether

    link/ether 52:54:00:1f:06:2a brd ff:ff:ff:ff:ff:ff
ip addr show dev dp0s8.40 | grep ether
    link/ether 52:54:00:1f:06:2a brd ff:ff:ff:ff:ff:ff
sudo ip link set dp0s8 addr 10:20:30:40:50:61
ip addr show dev dp0s8 | grep ether
    link/ether 10:20:30:40:50:61 brd ff:ff:ff:ff:ff:ff
ip addr show dev dp0s8.40 | grep ether
    link/ether 10:20:30:40:50:61 brd ff:ff:ff:ff:ff:ff

[  196.574789] 8021q: --- vlan_sync_address: id 40, for 10:20:30:40:50:61 on dp0s8.40 (from dp0
s8)
[  196.575004] --- vlan_sync_address: id 40, update to 10:20:30:40:50:61 on dp0s8.40 (from dp0s

sudo ip link set dp0s8 addr 52:54:00:1f:06:2a
ip addr show dev dp0s8 | grep ether

    link/ether 52:54:00:1f:06:2a brd ff:ff:ff:ff:ff:ff
ip addr show dev dp0s8.40 | grep ether
    link/ether 52:54:00:1f:06:2a brd ff:ff:ff:ff:ff:ff

[  265.683313] 8021q: --- vlan_sync_address: id 40, for 52:54:00:1f:06:2a on dp0s8.40 (from dp0
s8)
[  265.683534] --- vlan_sync_address: id 40, update to 52:54:00:1f:06:2a on dp0s8.40 (from dp0s

sudo ip link set dp0s8.40 addr 10:20:30:40:50:61
sudo ip link set dp0s8 addr 10:20:30:40:50:99
ip addr show dev dp0s8 | grep ether

    link/ether 10:20:30:40:50:99 brd ff:ff:ff:ff:ff:ff
ip addr show dev dp0s8.40 | grep ether
    link/ether 10:20:30:40:50:61 brd ff:ff:ff:ff:ff:ff

sudo ip link set dp0s8 addr 10:20:30:40:50:61

[ 5561.791222] --- vlan_dev_set_mac_address: id 40, for 10:20:30:40:50:61 on dp0s8
[ 5561.791225] --- vlan_dev_set_mac_address: id 40, call dev_uc_add for 10:20:30:40:50:61 on dp
0s8
[ 5561.791227] device dp0s8 entered promiscuous mode
[ 5601.050450] 8021q: --- vlan_sync_address: id 40, for 10:20:30:40:50:99 on dp0s8.40 (from dp0
s8)
[ 5630.258345] 8021q: --- vlan_sync_address: id 40, for 10:20:30:40:50:61 on dp0s8.40 (from dp0
s8)
[ 5630.258497] --- vlan_sync_address: id 40, call dev_uc_del for 10:20:30:40:50:61 on dp0s8
[ 5630.258499] --- __hw_addr_del_entry: refcount 0 for 10:20:30:40:50:61
[ 5630.258501] device dp0s8 left promiscuous mode

sudo ip link set dp0s8.40 addr 52:54:00:1f:06:2a
sudo ip link set dp0s8 addr 52:54:00:1f:06:2a

[ 5730.706196] --- vlan_dev_set_mac_address: id 40, for 52:54:00:1f:06:2a on dp0s8
[ 5730.706199] --- vlan_dev_set_mac_address: id 40, call dev_uc_add for 52:54:00:1f:06:2a on dp
0s8
[ 5730.706211] device dp0s8 entered promiscuous mode
[ 5737.378102] 8021q: --- vlan_sync_address: id 40, for 52:54:00:1f:06:2a on dp0s8.40 (from dp0
s8)
[ 5737.378236] --- vlan_sync_address: id 40, call dev_uc_del for 52:54:00:1f:06:2a on dp0s8
[ 5737.378238] --- __hw_addr_del_entry: refcount 0 for 52:54:00:1f:06:2a
[ 5737.378239] device dp0s8 left promiscuous mode

Quoted 4 lines

Also your subject line isn't formatted properly, it should be:
[PATCH net] vlan: Propagate MAC address changes properly.

Will resend with correct subject line, apologies for delay in replying.