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

[PATCH v5 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 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 v5 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 | 133 +++++++++++++++++++++++++++---------------
 1 file changed, 85 insertions(+), 48 deletions(-)

diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index afad90b9222a2..76c11e5260334 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.
@@ -979,7 +984,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 +1078,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 +1103,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))
@@ -1310,18 +1334,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 +1535,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 +1547,31 @@ 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);
 		eth_hw_addr_inherit(dev, lowerdev);
 	}
 
@@ -1581,7 +1603,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) {
+		macvlan_set_passthru(port);
+		list_add_rcu(&vlan->list, &port->vlans);
+	} 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 +1679,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 (vlan->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 +1928,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 v5 1/1] macvlan: allow source mode devices along with passthru

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917101044.2654867-1-t.martitz@fritz.com?part=1
---
macvlan: allow source mode devices along with passthru

This allows one passthru macvlan to coexist with source mode macvlans on
the same lower device, by converting several per-device decisions from the
port wide MACVLAN_F_PASSTHRU flag to vlan->mode and by moving the lower
device MAC restore from macvlan_port_destroy() into macvlan_uninit().

A few questions about the conversion below.

> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index afad90b9222a2..76c11e5260334 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)

[Severity: High]

macvlan_open() and macvlan_stop() are converted to the per-device
vlan->mode test here, but macvlan_change_rx_flags() still keys off the
port flag:

drivers/net/macvlan.c:macvlan_change_rx_flags() {
	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)
			dev_set_promiscuity(lowerdev,
					    dev->flags & IFF_PROMISC ? 1 : -1);
	}
}

Once a passthru device exists the port flag is set for every sibling, so
live IFF_PROMISC transitions of a source mode macvlan are dropped, while
macvlan_open() and macvlan_stop() still take and release the lower device
promiscuity reference based on dev->flags.

Can this unbalance lowerdev->promiscuity?

  ip link add pt link eth0 type macvlan mode passthru
  ip link add src0 link eth0 type macvlan mode source
  ip link set src0 promisc on
  ip link set src0 up	  macvlan_open() -> dev_set_promiscuity(lowerdev, 1)
  ip link set src0 promisc off	  macvlan_change_rx_flags() skips the -1
  ip link set src0 down	  macvlan_stop() sees IFF_PROMISC clear, no -1

That leaks a +1 on the physical device. The reverse order (up without
promisc, promisc on while up, then down) issues an unmatched
dev_set_promiscuity(lowerdev, -1), and __dev_set_promiscuity() computes
0 + (-1) as 0xFFFFFFFF, takes the "flags = old_flags | IFF_PROMISC"
branch and returns 0, so the device stays pinned promiscuous with no
error reported. If the passthru device held the only reference, the count
instead reaches 0, IFF_PROMISC is cleared and the hardware filter is
reprogrammed while the passthru macvlan still needs it.

Before this patch a passthru port could hold no other device, so the port
wide test in macvlan_change_rx_flags() was equivalent to the per-device
one. Should that call site be converted too?

> @@ -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;

[Severity: Medium]

The macvlan_addr_busy() call used to run for the passthru device as well,
and its macvlan_hash_lookup() half caught collisions with siblings on the
same port. Moving it inside "if (!passthru_dev)" drops that check for the
passthru device.

Can a passthru device now take the address of a live source mode sibling?
macvlan_set_mac_address() returns before its own busy check:

drivers/net/macvlan.c:macvlan_set_mac_address() {
	if (READ_ONCE(vlan->mode) == MACVLAN_MODE_PASSTHRU) {
		macvlan_set_addr_change(vlan->port);
		return dev_set_mac_address(vlan->lowerdev, addr, NULL);
	}

	if (macvlan_addr_busy(vlan->port, addr->__data))
		return -EADDRINUSE;

so with pt (passthru) and src0 (source) both up, "ip link set pt address
<src0 mac>" reprograms the lower device and comes back through
macvlan_device_event() -> macvlan_sync_address() with passthru_dev true,
where nothing validates it anymore.

Both macvlans then end up hashed under the same address, and src0 can no
longer be brought up because macvlan_addr_busy() now compares against
port->dev->dev_addr unconditionally:

drivers/net/macvlan.c:macvlan_open() {
	err = -EADDRINUSE;
	if (macvlan_addr_busy(vlan->port, dev->dev_addr))
		goto out;

> @@ -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.
> @@ -979,7 +984,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);
>  }

[Severity: High]

The restore now runs at passthru device lifetime, but the matching save
into port->perm_addr is still done once at port creation:

drivers/net/macvlan.c:macvlan_port_create() {
	ether_addr_copy(port->perm_addr, dev->dev_addr);

and it is only refreshed while a passthru device already exists:

drivers/net/macvlan.c:macvlan_device_event() {
	case NETDEV_CHANGEADDR:
		if (!macvlan_passthru(port))
			return NOTIFY_DONE;

Since a port can now be created by, and outlive, source mode only devices,
can this write a stale address onto the lower device?

  ip link add src0 link eth0 type macvlan mode source
	  macvlan_port_create() saves eth0 mac A into port->perm_addr
  ip link set eth0 address B
	  macvlan_device_event() returns NOTIFY_DONE, perm_addr stays A
  ip link add pt link eth0 type macvlan mode passthru
	  accepted now, eth_hw_addr_inherit() gives pt mac B
  ip link del pt
	  macvlan_uninit() sees B != A and sets eth0 back to A

eth0 ends up with an address the macvlan never installed. Should
port->perm_addr be re-captured next to the eth_hw_addr_inherit() call in
macvlan_common_newlink() when the passthru device is created?

Two related questions on the same block:

macvlan_uninit() is also reached from the register_netdevice() and
netdev_upper_dev_link() failure paths in macvlan_common_newlink(), for a
passthru device that never touched the lower MAC. Does the restore fire
there too?

macvlan_clear_passthru() clears MACVLAN_F_PASSTHRU but not
MACVLAN_F_ADDRCHANGE. In a port that now survives the passthru device, can
a stale addr change bit be observed by the next passthru device created on
that port?

[Severity: Low]

These are not bugs, but checkpatch will complain about a few things
introduced here:

  if(!ether_addr_equal(port->dev->dev_addr, port->perm_addr)) {

missing space after if.

In macvlan_sync_address() the return is indented by two extra tabs:

		if (macvlan_addr_busy(vlan->port, addr))
				return -EADDRINUSE;

In macvlan_common_newlink() the if branch has braces and the else branch
does not. There is also a typo, "passhtrough", in the new
macvlan_changelink() comment, and some of the new comment lines run past
80 columns.

[ ... ]

> @@ -1310,18 +1334,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 +1535,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 +1547,31 @@ 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);
>  		eth_hw_addr_inherit(dev, lowerdev);
>  	}
>  
> @@ -1581,7 +1603,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) {
> +		macvlan_set_passthru(port);
> +		list_add_rcu(&vlan->list, &port->vlans);
> +	} else
> +		list_add_tail_rcu(&vlan->list, &port->vlans);

[Severity: Low]

This isn't a bug as far as I can tell, but the ordering here is the
reverse of initialise-then-publish: macvlan_set_passthru() makes
macvlan_handle_frame() take the passthru branch before the device is on
port->vlans. On teardown the mirror window exists too, since
macvlan_dellink() does list_del_rcu() while the flag is only cleared later
in macvlan_uninit().

I could not find a functional consequence, because the reader bails out in
both windows:

drivers/net/macvlan.c:macvlan_handle_frame() {
	if (macvlan_passthru(port))
		vlan = list_first_or_null_rcu(&port->vlans,
					      struct macvlan_dev, list);
	else
		vlan = macvlan_hash_lookup(port, eth->h_dest);
	if (!vlan || READ_ONCE(vlan->mode) == MACVLAN_MODE_SOURCE)
		return RX_HANDLER_PASS;

and rtnl is held across register_netdevice() and this list_add_rcu(), so
the device cannot be up in the creation window. Would publishing the list
entry before setting the flag, and using READ_ONCE()/WRITE_ONCE() on
port->flags which macvlan_handle_frame() reads from softirq context, be
preferable anyway?

>  	update_port_bc_queue_len(vlan->port);
>  	netif_stacked_transfer_operstate(lowerdev, dev);
>  	linkwatch_fire_event(dev);
> @@ -1652,19 +1679,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 (vlan->mode == MACVLAN_MODE_PASSTHRU ||
> +			    macvlan_passthru(vlan->port))
> +				return -EINVAL;
> +			if (vlan->mode == MACVLAN_MODE_SOURCE)
> +				macvlan_flush_sources(vlan->port, vlan);
> +		}
>  	}

[Severity: High]

The old condition rejected both directions of a passthru transition. The
new one only rejects leaving passthru (vlan->mode == MACVLAN_MODE_PASSTHRU)
and any mode change inside an already-passthru port
(macvlan_passthru(vlan->port)). On a port that has no passthru device, is
switching a device into passthru still rejected?

  ip link add m0 link eth0 type macvlan mode vepa
  ip link set m0 up
  ip link set m0 type macvlan mode passthru

macvlan_validate() accepts MACVLAN_MODE_PASSTHRU, neither arm of the new
test matches, and the tail of macvlan_changelink() commits it:

	if (set_mode)
		WRITE_ONCE(vlan->mode, mode);

MACVLAN_F_PASSTHRU is never set and the device is never moved to the list
head, so the comment right above the check ("Passthrough mode can't be set
or cleared dynamically") and the invariant documented at the list_add_rcu()
above no longer hold. Since this patch keys the per-device paths off
vlan->mode, that device now behaves as passthru where it matters:

  ip link set m0 address <x>	macvlan_set_mac_address() takes the
				passthru branch and calls
				dev_set_mac_address(vlan->lowerdev, ...)

IFLA_MACVLAN_MODE has no rtnl_dev_link_net_capable() check on the lower
device netns, unlike IFLA_MACVLAN_BC_QUEUE_LEN and IFLA_MACVLAN_BC_CUTOFF,
and netif_set_mac_address() performs no namespace capability check, so can
CAP_NET_ADMIN in the netns owning a delegated macvlan rewrite the lower
device MAC this way?

  ip link set m0 down		macvlan_stop() takes the passthru branch,
				skipping dev_uc_del() and the allmulti
				release, and issues an unmatched
				dev_set_promiscuity(lowerdev, -1)

macvlan_uninit() would then also rewrite the lower MAC from
port->perm_addr, and macvlan_fdb_add()/macvlan_fdb_del() start accepting
unicast filters. Meanwhile macvlan_handle_frame() still uses the hash path
because the port flag stayed clear, so the device reports passthru but does
not receive as passthru.

Should the "mode == MACVLAN_MODE_PASSTHRU" half of the old test be kept?

>  
>  	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)

[ ... ]

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

[Thomas Martitz]

Hello,

Apologize for forgetting a changelog entry. I'm adding it inline and hopefully don't
forget include it in the next posting (if any).

BTW: I'll be attending at LPC in Prague. Would be great to have a chat with your guys there.

Am 17.09.26 um 12:02 schrieb 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 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
> - 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(-)
> 

Best regards

-- 
Thomas Martitz <t.martitz@fritz.com>
FRITZ! Technology GmbH, Berlin (Germany)