Re: [PATCH RESEND 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.
Full review at: https://sashiko.dev
---
macvlan: allow source mode devices along with passthru

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.

> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index c40fa331836bb..d28f9d905a84d 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c

[ ... ]

> @@ -1523,10 +1514,30 @@ 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: High]
While macvlan_common_newlink() prevents creating non-SOURCE devices on a
passthru port, macvlan_changelink() does not seem to have a similar check.

Can a user bypass this restriction by creating a SOURCE device and then using
changelink to modify its mode to VEPA, bypassing MAC isolation?

[Severity: High]
With source devices and passthru devices now coexisting on the same port,
functions like macvlan_open(), macvlan_stop(), and macvlan_sync_address()
check the port-wide macvlan_passthru(vlan->port) flag instead of the device
mode.

If a user brings down a SOURCE device on a passthru port, will macvlan_stop()
incorrectly take the passthru teardown path? This appears to skip dev_uc_del()
and call dev_set_promiscuity(..., -1) for a device that never incremented it.

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

[Severity: High]
If a PASSTHRU device is deleted while SOURCE devices still exist on the port,
macvlan_uninit() decrements port->count but does not clear MACVLAN_F_PASSTHRU.

Does this leave the port in a permanently broken state where new PASSTHRU
devices cannot be created, and remaining SOURCE devices continue to be
mistakenly treated as passthru?

>  		eth_hw_addr_inherit(dev, lowerdev);