macvlan: Deterministic ingress packet delivery

Changing the mac address when a macvlan device is up will leave the
device on the wrong hash chain making it impossible to receive
packets.

There is no checking of the mac address set on the macvlan.  Allowing
a misconfiguration to grab packets from the the underlying device or
another macvlan.

To resolve these problems I update the hash table of macvlans when the
mac address of a macvlan changes, and when updating the hash table
I verify that the new mac address is usable.

The result is well defined and predictable if not perfect handling of
mac vlan mac addresses.

To keep the code clear I have created a set of hash table maintenance
in macvlan so I am not open coding the hash function and the logic
needed to update the hash table all over the place.

Signed-off-by: Eric Biederman <[email protected]>
Acked-by: Patrick McHardy <[email protected]>
Signed-off-by: David S. Miller <[email protected]>

Author: Eric Biederman

drivers/net/macvlan.c

@@ -60,6 +60,47 @@ static struct macvlan_dev *macvlan_hash_lookup(const struct macvlan_port *port,
 	return NULL;
 }
 
+static void macvlan_hash_add(struct macvlan_dev *vlan)
+{
+	struct macvlan_port *port = vlan->port;
+	const unsigned char *addr = vlan->dev->dev_addr;
+
+	hlist_add_head_rcu(&vlan->hlist, &port->vlan_hash[addr[5]]);
+}
+
+static void macvlan_hash_del(struct macvlan_dev *vlan)
+{
+	hlist_del_rcu(&vlan->hlist);
+	synchronize_rcu();
+}
+
+static void macvlan_hash_change_addr(struct macvlan_dev *vlan,
+					const unsigned char *addr)
+{
+	macvlan_hash_del(vlan);
+	/* Now that we are unhashed it is safe to change the device
+	 * address without confusing packet delivery.
+	 */
+	memcpy(vlan->dev->dev_addr, addr, ETH_ALEN);
+	macvlan_hash_add(vlan);
+}
+
+static int macvlan_addr_busy(const struct macvlan_port *port,
+				const unsigned char *addr)
+{
+	/* Test to see if the specified multicast address is
+	 * currently in use by the underlying device or
+	 * another macvlan.
+	 */
+	if (memcmp(port->dev->dev_addr, addr, ETH_ALEN) == 0)
+		return 1;
+
+	if (macvlan_hash_lookup(port, addr))
+		return 1;
+
+	return 0;
+}
+
 static void macvlan_broadcast(struct sk_buff *skb,
 			      const struct macvlan_port *port)
 {
@@ -184,10 +225,13 @@ static const struct header_ops macvlan_hard_header_ops = {
 static int macvlan_open(struct net_device *dev)
 {
 	struct macvlan_dev *vlan = netdev_priv(dev);
-	struct macvlan_port *port = vlan->port;
 	struct net_device *lowerdev = vlan->lowerdev;
 	int err;
 
+	err = -EBUSY;
+	if (macvlan_addr_busy(vlan->port, dev->dev_addr))
+		goto out;
+
 	err = dev_unicast_add(lowerdev, dev->dev_addr, ETH_ALEN);
 	if (err < 0)
 		goto out;
@@ -196,8 +240,7 @@ static int macvlan_open(struct net_device *dev)
 		if (err < 0)
 			goto del_unicast;
 	}
-
-	hlist_add_head_rcu(&vlan->hlist, &port->vlan_hash[dev->dev_addr[5]]);
+	macvlan_hash_add(vlan);
 	return 0;
 
 del_unicast:
@@ -217,8 +260,7 @@ static int macvlan_stop(struct net_device *dev)
 
 	dev_unicast_delete(lowerdev, dev->dev_addr, ETH_ALEN);
 
-	hlist_del_rcu(&vlan->hlist);
-	synchronize_rcu();
+	macvlan_hash_del(vlan);
 	return 0;
 }
 
@@ -232,16 +274,21 @@ static int macvlan_set_mac_address(struct net_device *dev, void *p)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
-	if (!(dev->flags & IFF_UP))
-		goto out;
+	if (!(dev->flags & IFF_UP)) {
+		/* Just copy in the new address */
+		memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
+	} else {
+		/* Rehash and update the device filters */
+		if (macvlan_addr_busy(vlan->port, addr->sa_data))
+			return -EBUSY;
 
-	err = dev_unicast_add(lowerdev, addr->sa_data, ETH_ALEN);
-	if (err < 0)
-		return err;
-	dev_unicast_delete(lowerdev, dev->dev_addr, ETH_ALEN);
+		if ((err = dev_unicast_add(lowerdev, addr->sa_data, ETH_ALEN)))
+			return err;
 
-out:
-	memcpy(dev->dev_addr, addr->sa_data, ETH_ALEN);
+		dev_unicast_delete(lowerdev, dev->dev_addr, ETH_ALEN);
+
+		macvlan_hash_change_addr(vlan, addr->sa_data);
+	}
 	return 0;
 }