[PATCH v6 0/1] macvlan: allow source mode devices along with passthru

[PATCH v6 0/1] macvlan: allow source mode devices along with passthru

[Thomas Martitz]

Hello,

we're trying to solve a use case on our devices where two SoC are
connected on the same board, using the only available high-speed interface.

One SoC runs the main Linux system including the full routing stack
(FRITZ!OS) and the other SoC implements most of the GPON ONT side.

The high-speed interface is of course also used for the user traffic.
Therefore we must tell the inter-SoC traffic apart from the user traffic.

We achieve this by matching the well-known MAC address of the ONT SoC.
The user traffic passes through the ONT SoC without modifying MAC headers.
Now we would like to use macvlan (with source mode devices) on the main
SoC side for this but our routing stack requires the rx_handler to be
available. Therefore macvlan is currently not an option.

With this patch macvlan becomes an option because the current limitation
of either "one passthru device" or "any other configuration" is relaxed
for the combination of passthru and any number of source mode devices.

This allows us to configure a source mode device for the other SoC and
register an rx_handler for further processing on the passthru device.

Thanks in advance!

---
Changes in v6
- Prevent passthru and source mac from having the same MAC (if both are UP)
- Fix unbalanced promiscuity
- Really prevent changing anything to passthru
- Fix restoration of lowerdev MAC address

Changes in v5
- Properly respect "macvlan_addr_busy()" again for non-passthru  interfaces (and avoid calling anymore for passthru interfaces)
- Guard NETDEV_CHANGEADDR from messing with the lower device
  address during passthru interface removal
- restore unicast address (dev_add_uc/_del_uc) handling> Changes in v4

Changes in v4
- Prevent macvlan_restore_mac() from changing the MAC address
  of remaining source mode interfaces.
- Set MACVLAN_F_PASSTHRU on the macvlan_port only after adding
  the passthru interface truly succeeds.
- Changing existing interfaces to passthru mode shouldn't become
  allowed.

Changes in v3
- fix passthru port removal caused by passing the wrong 
  device to macvlan_port_release_mac(). This was also
  detected by syzbot.
- macvlan_port_release_mac() is now named macvlan_restore_mac()
  and gets passed a "struct macvlan_port" directly.
- Link to v2: https://lore.kernel.org/netdev/20260709100512.1383421-1-t.martitz@fritz.com/

Changes in v2:
- changed several port-wide checks (macvlan_passthru()) to 
  per-interface checks (vlan->mode == vlan->mode == MACVLAN_MODE_PASSTHRU)
- correctly handle removing the passthru interface when there are still
  source interfaces
- Link to initial posting: https://lore.kernel.org/netdev/20260612092345.2352255-1-t.martitz@fritz.com/
---

Thomas Martitz (1):
  macvlan: allow source mode devices along with passthru

 drivers/net/macvlan.c | 107 ++++++++++++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 34 deletions(-)

-- 
2.54.0

[PATCH v6 1/1] macvlan: allow source mode devices along with passthru

[Thomas Martitz]

This allows for configurations where there are a few
known senders in the system (e.g. multiple SoCs on the same
board) along with unlimited external senders.

The source mode devices represent the known senders while
all external senders terminate on passthru device.

Although you can still receive packets on the lower device
without the need for the passthru vlan device, there
are use cases where you need additional packet processing
in the pipeline that hooks via rx_handler. With this the
rx_handler can be attached to the passthru device while
macvlan itself remains attached to the lower device.

We use this to use the same physical link for inter-SoC
networking and external networking. Some of our chips
have no other viable link for inter-SoC traffic.

Signed-off-by: Thomas Martitz <t.martitz@fritz.com>
---
 drivers/net/macvlan.c | 146 +++++++++++++++++++++++++++---------------
 1 file changed, 93 insertions(+), 53 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index afad90b9222a2..4af10cc517993 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -83,6 +83,11 @@ static inline void macvlan_set_passthru(struct macvlan_port *port)
 	port->flags |= MACVLAN_F_PASSTHRU;
 }
 
+static inline void macvlan_clear_passthru(struct macvlan_port *port)
+{
+	port->flags &= ~MACVLAN_F_PASSTHRU;
+}
+
 static inline bool macvlan_addr_change(const struct macvlan_port *port)
 {
 	return port->flags & MACVLAN_F_ADDRCHANGE;
@@ -217,8 +222,7 @@ static bool macvlan_addr_busy(const struct macvlan_port *port,
 	 * currently in use by the underlying device or
 	 * another macvlan.
 	 */
-	if (!macvlan_passthru(port) && !macvlan_addr_change(port) &&
-	    ether_addr_equal_64bits(port->dev->dev_addr, addr))
+	if (ether_addr_equal_64bits(port->dev->dev_addr, addr))
 		return true;
 
 	if (macvlan_hash_lookup(port, addr))
@@ -637,7 +641,7 @@ static int macvlan_open(struct net_device *dev)
 	struct net_device *lowerdev = vlan->lowerdev;
 	int err;
 
-	if (macvlan_passthru(vlan->port)) {
+	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
 		if (!(vlan->flags & MACVLAN_FLAG_NOPROMISC)) {
 			err = dev_set_promiscuity(lowerdev, 1);
 			if (err < 0)
@@ -712,7 +716,7 @@ static int macvlan_stop(struct net_device *dev)
 	dev_uc_unsync(lowerdev, dev);
 	dev_mc_unsync(lowerdev, dev);
 
-	if (macvlan_passthru(vlan->port)) {
+	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
 		if (!(vlan->flags & MACVLAN_FLAG_NOPROMISC))
 			dev_set_promiscuity(lowerdev, -1);
 		goto hash_del;
@@ -737,17 +741,18 @@ static int macvlan_sync_address(struct net_device *dev,
 	struct macvlan_dev *vlan = netdev_priv(dev);
 	struct net_device *lowerdev = vlan->lowerdev;
 	struct macvlan_port *port = vlan->port;
+	bool passthru_dev = vlan->mode == MACVLAN_MODE_PASSTHRU;
 	int err;
 
 	if (!(dev->flags & IFF_UP)) {
 		/* Just copy in the new address */
 		eth_hw_addr_set(dev, addr);
 	} else {
-		/* Rehash and update the device filters */
-		if (macvlan_addr_busy(vlan->port, addr))
-			return -EADDRINUSE;
+		if (!passthru_dev) {
+			/* Rehash and update the device filters */
+			if (macvlan_addr_busy(vlan->port, addr))
+					return -EADDRINUSE;
 
-		if (!macvlan_passthru(port)) {
 			err = dev_uc_add(lowerdev, addr);
 			if (err)
 				return err;
@@ -757,7 +762,7 @@ static int macvlan_sync_address(struct net_device *dev,
 
 		macvlan_hash_change_addr(vlan, addr);
 	}
-	if (macvlan_passthru(port) && !macvlan_addr_change(port)) {
+	if (passthru_dev && !macvlan_addr_change(port)) {
 		/* Since addr_change isn't set, we are here due to lower
 		 * device change.  Save the lower-dev address so we can
 		 * restore it later.
@@ -781,14 +786,17 @@ static int macvlan_set_mac_address(struct net_device *dev, void *p)
 	if (ether_addr_equal(dev->dev_addr, addr->__data))
 		return 0;
 
+	if (macvlan_addr_busy(vlan->port, addr->__data))
+		return -EADDRINUSE;
+
 	if (READ_ONCE(vlan->mode) == MACVLAN_MODE_PASSTHRU) {
 		macvlan_set_addr_change(vlan->port);
+		/* macvlan_sync_address() via netdev notifier,
+		 * to keep passthru and lower in sync.
+		 */
 		return dev_set_mac_address(vlan->lowerdev, addr, NULL);
 	}
 
-	if (macvlan_addr_busy(vlan->port, addr->__data))
-		return -EADDRINUSE;
-
 	return macvlan_sync_address(dev, addr->__data);
 }
 
@@ -800,7 +808,7 @@ static void macvlan_change_rx_flags(struct net_device *dev, int change)
 	if (dev->flags & IFF_UP) {
 		if (change & IFF_ALLMULTI)
 			dev_set_allmulti(lowerdev, dev->flags & IFF_ALLMULTI ? 1 : -1);
-		if (!macvlan_passthru(vlan->port) && change & IFF_PROMISC)
+		if (READ_ONCE(vlan->mode) != MACVLAN_MODE_PASSTHRU && change & IFF_PROMISC)
 			dev_set_promiscuity(lowerdev,
 					    dev->flags & IFF_PROMISC ? 1 : -1);
 
@@ -979,7 +987,26 @@ static void macvlan_uninit(struct net_device *dev)
 
 	macvlan_flush_sources(port, vlan);
 	port->count -= 1;
-	if (!port->count)
+
+	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
+		/* If the lower device address has been changed by passthru
+		 * macvlan, put it back.
+		 *
+		 * Caution: This triggers NETDEV_CHANGEADDR on
+		 * the lower device. But we're in the green because
+		 * the passthru interface is already off the list.
+		 */
+		if (!ether_addr_equal(port->dev->dev_addr, port->perm_addr)) {
+			struct sockaddr_storage ss;
+
+			ss.ss_family = port->dev->type;
+			memcpy(&ss.__data, port->perm_addr, port->dev->addr_len);
+			dev_set_mac_address(port->dev, &ss, NULL);
+		}
+		macvlan_clear_passthru(port);
+	}
+
+	if (port->count == 0)
 		macvlan_port_destroy(port->dev);
 }
 
@@ -1054,7 +1081,7 @@ static int macvlan_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
 	/* Support unicast filter only on passthru devices.
 	 * Multicast filter should be allowed on all devices.
 	 */
-	if (!macvlan_passthru(vlan->port) && is_unicast_ether_addr(addr))
+	if (vlan->mode != MACVLAN_MODE_PASSTHRU && is_unicast_ether_addr(addr))
 		return -EOPNOTSUPP;
 
 	if (flags & NLM_F_REPLACE)
@@ -1079,7 +1106,7 @@ static int macvlan_fdb_del(struct ndmsg *ndm, struct nlattr *tb[],
 	/* Support unicast filter only on passthru devices.
 	 * Multicast filter should be allowed on all devices.
 	 */
-	if (!macvlan_passthru(vlan->port) && is_unicast_ether_addr(addr))
+	if (vlan->mode != MACVLAN_MODE_PASSTHRU && is_unicast_ether_addr(addr))
 		return -EOPNOTSUPP;
 
 	if (is_unicast_ether_addr(addr))
@@ -1268,7 +1295,6 @@ static int macvlan_port_create(struct net_device *dev)
 		return -ENOMEM;
 
 	port->dev = dev;
-	ether_addr_copy(port->perm_addr, dev->dev_addr);
 	INIT_LIST_HEAD(&port->vlans);
 	for (i = 0; i < MACVLAN_HASH_SIZE; i++)
 		INIT_HLIST_HEAD(&port->vlan_hash[i]);
@@ -1310,18 +1336,6 @@ static void macvlan_port_destroy(struct net_device *dev)
 		kfree_skb(skb);
 	}
 
-	/* If the lower device address has been changed by passthru
-	 * macvlan, put it back.
-	 */
-	if (macvlan_passthru(port) &&
-	    !ether_addr_equal(port->dev->dev_addr, port->perm_addr)) {
-		struct sockaddr_storage ss;
-
-		ss.ss_family = port->dev->type;
-		memcpy(&ss.__data, port->perm_addr, port->dev->addr_len);
-		dev_set_mac_address(port->dev, &ss, NULL);
-	}
-
 	kfree(port);
 }
 
@@ -1523,15 +1537,6 @@ int macvlan_common_newlink(struct net_device *dev,
 	}
 	port = macvlan_port_get_rtnl(lowerdev);
 
-	/* Only 1 macvlan device can be created in passthru mode */
-	if (macvlan_passthru(port)) {
-		/* The macvlan port must be not created this time,
-		 * still goto destroy_macvlan_port for readability.
-		 */
-		err = -EINVAL;
-		goto destroy_macvlan_port;
-	}
-
 	vlan->lowerdev = lowerdev;
 	vlan->dev      = dev;
 	vlan->port     = port;
@@ -1544,12 +1549,32 @@ int macvlan_common_newlink(struct net_device *dev,
 	if (data && data[IFLA_MACVLAN_FLAGS])
 		vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
 
+	/* Only 1 macvlan device can be created in passthru mode. There may be
+	 * additional source mode devices but nothing else at the moment.
+	 *
+	 * First check if adding a source mode device to an existing passthru vlan.
+	 */
+	if (macvlan_passthru(port) && vlan->mode != MACVLAN_MODE_SOURCE) {
+		/* The macvlan port must be not created this time,
+		 * still goto destroy_macvlan_port for readability.
+		 */
+		err = -EINVAL;
+		goto destroy_macvlan_port;
+	}
+
+	/* Now check if adding a passthru device to an existing set of source mode
+	 * devices.
+	 */
 	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
-		if (port->count) {
-			err = -EINVAL;
-			goto destroy_macvlan_port;
+		struct macvlan_dev *p;
+
+		list_for_each_entry(p, &port->vlans, list) {
+			if (p->mode != MACVLAN_MODE_SOURCE) {
+				err = -EINVAL;
+				goto destroy_macvlan_port;
+			}
 		}
-		macvlan_set_passthru(port);
+		ether_addr_copy(port->perm_addr, lowerdev->dev_addr);
 		eth_hw_addr_inherit(dev, lowerdev);
 	}
 
@@ -1581,7 +1606,12 @@ int macvlan_common_newlink(struct net_device *dev,
 	if (err)
 		goto unregister_netdev;
 
-	list_add_tail_rcu(&vlan->list, &port->vlans);
+	/* macvlan_handle_frame expects the (one and only) passthru device first. */
+	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
+		list_add_rcu(&vlan->list, &port->vlans);
+		macvlan_set_passthru(port);
+	} else
+		list_add_tail_rcu(&vlan->list, &port->vlans);
 	update_port_bc_queue_len(vlan->port);
 	netif_stacked_transfer_operstate(lowerdev, dev);
 	linkwatch_fire_event(dev);
@@ -1652,19 +1682,23 @@ static int macvlan_changelink(struct net_device *dev,
 	if (data && data[IFLA_MACVLAN_MODE]) {
 		set_mode = true;
 		mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
-		/* Passthrough mode can't be set or cleared dynamically */
-		if ((mode == MACVLAN_MODE_PASSTHRU) !=
-		    (vlan->mode == MACVLAN_MODE_PASSTHRU))
-			return -EINVAL;
-		if (vlan->mode == MACVLAN_MODE_SOURCE &&
-		    vlan->mode != mode)
-			macvlan_flush_sources(vlan->port, vlan);
+		if (mode != vlan->mode) {
+			/* Passthrough mode can't be set or cleared dynamically,
+			 * regardless of existing source interfaces. Furthermore, source
+			 * interfaces can't switch modes within a passhtrough port.
+			 */
+			if (mode == MACVLAN_MODE_PASSTHRU ||
+			    macvlan_passthru(vlan->port))
+				return -EINVAL;
+			if (vlan->mode == MACVLAN_MODE_SOURCE)
+				macvlan_flush_sources(vlan->port, vlan);
+		}
 	}
 
 	if (data && data[IFLA_MACVLAN_FLAGS]) {
 		__u16 flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
 		bool promisc = (flags ^ vlan->flags) & MACVLAN_FLAG_NOPROMISC;
-		if (macvlan_passthru(vlan->port) && promisc) {
+		if (vlan->mode == MACVLAN_MODE_PASSTHRU && promisc) {
 			int err;
 
 			if (flags & MACVLAN_FLAG_NOPROMISC)
@@ -1897,8 +1931,14 @@ static int macvlan_device_event(struct notifier_block *unused,
 		vlan = list_first_entry_or_null(&port->vlans,
 						struct macvlan_dev,
 						list);
-
-		if (vlan && macvlan_sync_address(vlan->dev, dev->dev_addr))
+		/* Validate the interface mode again because we're potentially
+		 * triggered upon removal of the passthru interface. In which
+		 * case it is already off the list (and has restored the lower
+		 * device mac already).
+		 */
+		if (!vlan || vlan->mode != MACVLAN_MODE_PASSTHRU)
+			return NOTIFY_DONE;
+		if (macvlan_sync_address(vlan->dev, dev->dev_addr))
 			return NOTIFY_BAD;
 
 		break;
-- 
2.55.0

Re: [PATCH v6 1/1] macvlan: allow source mode devices along with passthru

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
macvlan: allow source mode devices along with passthru

This patch lets source-mode macvlan devices share a port with a single
passthru macvlan. Known senders end on the source devices and all other
traffic goes to the passthru device. The lower MAC restore moves from
macvlan_port_destroy() to macvlan_uninit() of the passthru device, and
several macvlan_passthru(port) checks become per-device vlan->mode checks.

> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index afad90b9222a2..4af10cc517993 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -83,6 +83,11 @@ static inline void macvlan_set_passthru(struct macvlan_port *port)
>  	port->flags |= MACVLAN_F_PASSTHRU;
>  }
>  
> +static inline void macvlan_clear_passthru(struct macvlan_port *port)
> +{
> +	port->flags &= ~MACVLAN_F_PASSTHRU;
> +}
> +

[Severity: Low]
With this patch, MACVLAN_F_PASSTHRU is set and cleared at runtime while
the rx_handler stays registered for the remaining source devices.
macvlan_set_passthru() and macvlan_clear_passthru() are both plain
read-modify-writes under RTNL. macvlan_handle_frame() reads the flag with
a plain load in softirq:

	if (macvlan_passthru(port))
		vlan = list_first_or_null_rcu(&port->vlans,
					      struct macvlan_dev, list);

Nothing orders the flag against the port->vlans update. The reader seems
to handle both transient states, because the next check passes the frame
when the list head is NULL or a source device:

	if (!vlan || READ_ONCE(vlan->mode) == MACVLAN_MODE_SOURCE)
		return RX_HANDLER_PASS;

The plain-access race on port->flags already existed through
MACVLAN_F_ADDRCHANGE. Would READ_ONCE()/WRITE_ONCE() on port->flags be a
reasonable cleanup now that there is another runtime writer?

[ ... ]

> @@ -737,17 +741,18 @@ static int macvlan_sync_address(struct net_device *dev,
>  	struct macvlan_dev *vlan = netdev_priv(dev);
>  	struct net_device *lowerdev = vlan->lowerdev;
>  	struct macvlan_port *port = vlan->port;
> +	bool passthru_dev = vlan->mode == MACVLAN_MODE_PASSTHRU;
>  	int err;
>  
>  	if (!(dev->flags & IFF_UP)) {
>  		/* Just copy in the new address */
>  		eth_hw_addr_set(dev, addr);
>  	} else {
> -		/* Rehash and update the device filters */
> -		if (macvlan_addr_busy(vlan->port, addr))
> -			return -EADDRINUSE;
> +		if (!passthru_dev) {
> +			/* Rehash and update the device filters */
> +			if (macvlan_addr_busy(vlan->port, addr))
> +					return -EADDRINUSE;

[Severity: Low]
For the passthru device, this now skips macvlan_addr_busy() entirely. That
includes the vlan_hash lookup the old code still did on passthru ports.
The new check in macvlan_set_mac_address() only covers changes made
through the passthru device.

What happens when the change starts on the lower device? For example,
setting eth0's MAC to the MAC of a source-mode macvlan that is up:

netif_set_mac_address()
  NETDEV_CHANGEADDR
    macvlan_device_event()
      macvlan_sync_address()
        macvlan_hash_change_addr()

Wouldn't this put the passthru device into port->vlan_hash with the same
MAC the source device already has there?

The same seems to happen when the passthru device is down.
eth_hw_addr_set() copies the address, and macvlan_open() later hashes it
with no busy check.

After that, lookups through macvlan_hash_lookup(), such as the h_source
lookup on the multicast rx path, depend on hash order. The source device
also can't be reopened, because its address now equals the lower's.

netif_set_mac_address() ignores the NETDEV_CHANGEADDR notifier return
value, so the old NOTIFY_BAD path would not have vetoed this either. Would
a NETDEV_PRE_CHANGEADDR check be the right place to reject it?

[ ... ]

> @@ -781,14 +786,17 @@ static int macvlan_set_mac_address(struct net_device *dev, void *p)
>  	if (ether_addr_equal(dev->dev_addr, addr->__data))
>  		return 0;
>  
> +	if (macvlan_addr_busy(vlan->port, addr->__data))
> +		return -EADDRINUSE;
> +
>  	if (READ_ONCE(vlan->mode) == MACVLAN_MODE_PASSTHRU) {
>  		macvlan_set_addr_change(vlan->port);
> +		/* macvlan_sync_address() via netdev notifier,
> +		 * to keep passthru and lower in sync.
> +		 */
>  		return dev_set_mac_address(vlan->lowerdev, addr, NULL);
>  	}

[Severity: Medium]
macvlan_set_addr_change() is called before
dev_set_mac_address(vlan->lowerdev, ...). That call can fail, for example
with -EBUSY from eth_prepare_mac_addr_change() on a running lower device
without IFF_LIVE_ADDR_CHANGE, or through a NETDEV_PRE_CHANGEADDR veto. In
that case netif_set_mac_address() returns before NETDEV_CHANGEADDR, and
nothing clears MACVLAN_F_ADDRCHANGE.

Before this patch, the port and the flag went away together with the only
passthru device. Now source devices keep the port alive.
macvlan_uninit() only calls macvlan_clear_passthru().
macvlan_common_newlink() resets port->perm_addr for a new passthru device,
but not the flag.

Can the following sequence restore an outdated MAC on the lower device?

1. ip link set <passthru> address X fails, leaving ADDRCHANGE set.
2. The passthru device is deleted while source devices still exist.
3. A new passthru device is created, which copies the current lower MAC
   into perm_addr.
4. The lower MAC is changed directly. macvlan_sync_address() sees the
   stale flag and does not save the new address:

	if (passthru_dev && !macvlan_addr_change(port)) {

5. The passthru device is deleted. macvlan_uninit() sees
   dev_addr != perm_addr and writes the old MAC back.

Should the flag be cleared when dev_set_mac_address() fails, or when the
passthru device goes away?

[ ... ]

> @@ -979,7 +987,26 @@ static void macvlan_uninit(struct net_device *dev)
>  
>  	macvlan_flush_sources(port, vlan);
>  	port->count -= 1;
> -	if (!port->count)
> +
> +	if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
> +		/* If the lower device address has been changed by passthru
> +		 * macvlan, put it back.
> +		 *
> +		 * Caution: This triggers NETDEV_CHANGEADDR on
> +		 * the lower device. But we're in the green because
> +		 * the passthru interface is already off the list.
> +		 */
> +		if (!ether_addr_equal(port->dev->dev_addr, port->perm_addr)) {
> +			struct sockaddr_storage ss;
> +
> +			ss.ss_family = port->dev->type;
> +			memcpy(&ss.__data, port->perm_addr, port->dev->addr_len);
> +			dev_set_mac_address(port->dev, &ss, NULL);
> +		}

[Severity: Medium]
Can this put a MAC on the lower device that a remaining source-mode
macvlan already uses? Consider this sequence:

1. The lower device has address A. A passthru device is created, so
   perm_addr = A.
2. The passthru address is set to B. The lower device changes to B, and
   perm_addr stays A because ADDRCHANGE is set.
3. A source-mode macvlan is created with IFLA_ADDRESS = A.
   macvlan_common_newlink() does no busy check.
4. The source device is brought up. In macvlan_open(),
   macvlan_addr_busy(A) only compares against the current lower address
   (B) and vlan_hash, so it succeeds and calls dev_uc_add(lower, A).
5. The passthru device is deleted. This code calls
   dev_set_mac_address(lower, A) without checking the macvlans that are
   left.

The lower device and the source device now share A. The next down/up of
the source device then fails in macvlan_open() with -EADDRINUSE, because
macvlan_addr_busy() hits this check:

	if (ether_addr_equal_64bits(port->dev->dev_addr, addr))
		return true;

[ ... ]

> @@ -1544,12 +1549,32 @@ int macvlan_common_newlink(struct net_device *dev,
>  	if (data && data[IFLA_MACVLAN_FLAGS])
>  		vlan->flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
>  
> +	/* Only 1 macvlan device can be created in passthru mode. There may be
> +	 * additional source mode devices but nothing else at the moment.
> +	 *
> +	 * First check if adding a source mode device to an existing passthru vlan.
> +	 */
> +	if (macvlan_passthru(port) && vlan->mode != MACVLAN_MODE_SOURCE) {
> +		/* The macvlan port must be not created this time,
> +		 * still goto destroy_macvlan_port for readability.
> +		 */
> +		err = -EINVAL;
> +		goto destroy_macvlan_port;
> +	}

[Severity: Medium]
Source-mode devices are now allowed on a passthru port, so the passthru
device no longer gets all traffic on the lower device. Are there places
outside macvlan.c that still assume it does?

In drivers/net/ethernet/mellanox/mlx5/core/en/rep/tc.c,
mlx5e_rep_check_indr_block_supported() offloads tc ingress filters bound
to a macvlan onto the real lower representor whenever this returns true:

mlx5e_rep_macvlan_mode_supported() {
	...
	return macvlan->mode == MACVLAN_MODE_PASSTHRU;
}

In software, macvlan_handle_frame() calls macvlan_forward_source() before
the passthru lookup. With MACVLAN_FLAG_NODST, known-sender frames are
consumed there and never reach the passthru device.

A hardware filter installed for the passthru macvlan could still match
frames entering the representor, including the inter-SoC ones. It could
then drop or redirect them before the source devices see them. I could
not confirm from the mlx5 flow steering code that the offloaded rule
matches every packet from the vport. If it does, hardware and software
would behave differently here. Is that a concern?

Also, the uAPI comment in include/uapi/linux/if_link.h still says:

	MACVLAN_MODE_PASSTHRU = 8,/* take over the underlying device */

Should that be updated as well?

[ ... ]

> @@ -1652,19 +1682,23 @@ static int macvlan_changelink(struct net_device *dev,
>  	if (data && data[IFLA_MACVLAN_MODE]) {
>  		set_mode = true;
>  		mode = nla_get_u32(data[IFLA_MACVLAN_MODE]);
> -		/* Passthrough mode can't be set or cleared dynamically */
> -		if ((mode == MACVLAN_MODE_PASSTHRU) !=
> -		    (vlan->mode == MACVLAN_MODE_PASSTHRU))
> -			return -EINVAL;
> -		if (vlan->mode == MACVLAN_MODE_SOURCE &&
> -		    vlan->mode != mode)
> -			macvlan_flush_sources(vlan->port, vlan);
> +		if (mode != vlan->mode) {
> +			/* Passthrough mode can't be set or cleared dynamically,
> +			 * regardless of existing source interfaces. Furthermore, source
> +			 * interfaces can't switch modes within a passhtrough port.
> +			 */
> +			if (mode == MACVLAN_MODE_PASSTHRU ||
> +			    macvlan_passthru(vlan->port))
> +				return -EINVAL;

[Severity: Low]
This adds a new restriction: while the port is in passthru, any mode
change of a source-mode device returns -EINVAL. The commit message does
not mention this or a few other user-visible changes:

- The lower MAC restore moved from macvlan_port_destroy() to
  macvlan_uninit() of the passthru device. It now runs even while source
  devices stay on the port.

- macvlan_forward_source() only consumes the frame when
  MACVLAN_FLAG_NODST is set. Without it, known-sender frames are cloned to
  the source device and also delivered to the passthru device.

The commit message says:

  "The source mode devices represent the known senders while
  all external senders terminate on passthru device."

That split seems to depend on NODST being set.

Documentation/networking/device_drivers/ethernet/intel/ice.rst also still
says:

  "In passthru mode, you can only set up one MACVLAN device."

Could the commit message and that document be updated to describe these
changes?

> +			if (vlan->mode == MACVLAN_MODE_SOURCE)
> +				macvlan_flush_sources(vlan->port, vlan);
> +		}
>  	}
>  
>  	if (data && data[IFLA_MACVLAN_FLAGS]) {
>  		__u16 flags = nla_get_u16(data[IFLA_MACVLAN_FLAGS]);
>  		bool promisc = (flags ^ vlan->flags) & MACVLAN_FLAG_NOPROMISC;
> -		if (macvlan_passthru(vlan->port) && promisc) {
> +		if (vlan->mode == MACVLAN_MODE_PASSTHRU && promisc) {
>  			int err;
>  
>  			if (flags & MACVLAN_FLAG_NOPROMISC)

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch.
macvlan_changelink() changes the lower device's promiscuity count whenever
MACVLAN_FLAG_NOPROMISC flips on the passthru device. It does not check
dev->flags & IFF_UP. macvlan_open() and macvlan_stop() only take or drop
that reference while the device is up.

Setting nopromisc on a passthru device that is down calls
dev_set_promiscuity(lowerdev, -1) with no matching +1. That either drops a
reference owned by another user, or wraps dev->promiscuity from 0 in
__dev_set_promiscuity() and leaves IFF_PROMISC set. Clearing nopromisc
while down takes a +1 that is never released.

Should this be gated on IFF_UP? For the passthru device, the patch's
change from macvlan_passthru(vlan->port) to
vlan->mode == MACVLAN_MODE_PASSTHRU is equivalent, so the behavior is
unchanged.

[ ... ]
-- 
pw-bot: cr