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

[Thomas Martitz]

Hi Simon,

Am 08.07.26 um 18:43 schrieb 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://sashiko.dev

Thanks for feedback. The remarks are valid, I agree. I also begin to better
understand the macvlan code so I'm a bit more confident now about which of
the existing "macvlan_passthru" checks need treatment. I'll send an updated
patch.

General question though: The initial posting triggered an AI review (I see
that now, I didn't know about https://sashiko.dev before). But it did not
trigger an email. Can you explain why? Because I accidentally dropped the RFC
tag from the mail subject?

Best regards.


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


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