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

[Simon Horman <horms@kernel.org>]

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

This change permits combining one passthru macvlan with one or more
source-mode macvlans on the same lower device, so that an rx_handler can
be attached to the passthru device while source-mode devices continue to
represent known senders.

> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index c40fa331836b..635ed01abeb6 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c

[ ... ]

> @@ -964,6 +969,21 @@ static int macvlan_init(struct net_device *dev)
>  	return 0;
>  }
>  
> +static void macvlan_restore_mac(struct macvlan_port *port)
> +{
> +	/* 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);
> +	}
> +}
> +
>  static void macvlan_uninit(struct net_device *dev)
>  {
>  	struct macvlan_dev *vlan = netdev_priv(dev);
> @@ -973,8 +993,19 @@ static void macvlan_uninit(struct net_device *dev)
>  
>  	macvlan_flush_sources(port, vlan);
>  	port->count -= 1;
> -	if (!port->count)
> -		macvlan_port_destroy(port->dev);
> +	if (port->count) {
> +		/* In case of remaining source interfaces undo
> +		 * passthru-specific properties.
> +		 */
> +		if (vlan->mode == MACVLAN_MODE_PASSTHRU) {
> +			/* Order is important, do not trigger macvlan_device_event(). */
> +			macvlan_clear_passthru(port);
> +			macvlan_restore_mac(port);

[High]
When the last passthru macvlan is removed while source-mode siblings
remain, does macvlan_restore_mac() actually restore the lower device's
MAC in this ordering?

macvlan_clear_passthru() is called first, which drops MACVLAN_F_PASSTHRU:

    static inline void macvlan_clear_passthru(struct macvlan_port *port)
    {
        port->flags &= ~MACVLAN_F_PASSTHRU;
    }

Then macvlan_restore_mac() runs, but its body is gated on
macvlan_passthru(port):

    static void macvlan_restore_mac(struct macvlan_port *port)
    {
        if (macvlan_passthru(port) &&
            !ether_addr_equal(port->dev->dev_addr, port->perm_addr)) {
            ...
            dev_set_mac_address(port->dev, &ss, NULL);
        }
    }

Since the flag has just been cleared, does the guard evaluate to false
here, causing dev_set_mac_address() to be skipped and the lower device
to retain the passthru MAC instead of port->perm_addr?

The pre-patch macvlan_port_destroy() path did not clear the flag first,
so the restore actually took effect there.

>  		}
> +		return;
> +	}
> +
> +	macvlan_port_destroy(port->dev);
>  }
>  
>  static void macvlan_dev_get_stats64(struct net_device *dev,

[ ... ]

> @@ -1623,19 +1659,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);
> +		}
>  	}

[High]
Does this new check still reject setting an existing non-passthru vlan
to MACVLAN_MODE_PASSTHRU when the port has no passthru vlan yet?

The comment still says "Passthrough mode can't be set or cleared
dynamically", but the condition:

    if (vlan->mode == MACVLAN_MODE_PASSTHRU ||
        macvlan_passthru(vlan->port))
        return -EINVAL;

only catches clearing passthru on a passthru vlan, and any mode change
on a vlan whose port already has passthru.

If vlan->mode is one of VEPA/BRIDGE/PRIVATE/SOURCE and the request is
mode == MACVLAN_MODE_PASSTHRU on a port that has no passthru vlan yet,
does execution fall through? Later in the same function:

    if (set_mode)
        vlan->mode = mode;

promotes vlan->mode to PASSTHRU, but nothing calls
macvlan_set_passthru(port), nothing performs eth_hw_addr_inherit(dev,
lowerdev), and no passthru promiscuity accounting is done.

After this, does the vlan report mode == PASSTHRU while the port flag
MACVLAN_F_PASSTHRU stays clear? Downstream code splits between the two
predicates:

  - macvlan_open, macvlan_stop, macvlan_fdb_add, macvlan_fdb_del, and
    the IFLA_MACVLAN_FLAGS branch here read vlan->mode ==
    MACVLAN_MODE_PASSTHRU.

  - macvlan_device_event NETDEV_CHANGEADDR, macvlan_addr_busy,
    macvlan_change_rx_flags, macvlan_sync_address and
    macvlan_handle_frame read macvlan_passthru(port).

Does this then produce visible symptoms such as:

  - macvlan_open calling dev_set_promiscuity(lowerdev, 1) while the
    lower device's MAC was never inherited to the vlan;
  - macvlan_addr_busy no longer treating the port as passthru, allowing
    a sibling with the lowerdev's address;
  - macvlan_handle_frame not delivering to this vlan via
    list_first_or_null_rcu(&port->vlans) because the port flag is
    clear?

Would a condition that also rejects the direction into passthru be more
consistent with the comment, e.g.:

    if (vlan->mode == MACVLAN_MODE_PASSTHRU ||
        mode == MACVLAN_MODE_PASSTHRU ||
        macvlan_passthru(vlan->port))
        return -EINVAL;

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