Re: [PATCH net-next 03/12] net: bridge: add VLAN support to bridge destinations

[Nikolay Aleksandrov <razor@blackwall.org>]

On 01/10/2026 14:59, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] Baseline decision: false positive.
> 
> --
> 
> net: bridge: add VLAN support to bridge destinations
> 
> This commit sets bit 0 of struct net_bridge_dst to mark a port-VLAN
> pointer, so it can be told apart from a bridge port pointer. It adds
> helpers that build VLAN destinations and decode them back into a port and
> a VLAN. Nothing creates a VLAN destination yet.
> 
>> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
>> index bbb59b53e7452..2bf7f4429a3b2 100644
>> --- a/net/bridge/br_private.h
>> +++ b/net/bridge/br_private.h
> 
> [ ... ]
> 
>> @@ -702,10 +704,56 @@ br_port_to_dst(const struct net_bridge_port *p)
>>   	return dst;
>>   }
>>   
>> +static inline struct net_bridge_dst
>> +br_vlan_to_dst(const struct net_bridge_vlan *v)
>> +{
>> +	struct net_bridge_dst dst;
>> +
>> +	ACCESS_PRIVATE(&dst, value) = (unsigned long)v | BR_DST_VLAN_TAG;
>> +
>> +	return dst;
>> +}
>> +
>> +static inline void br_dst_decode(struct net_bridge_dst dst,
>> +				 struct net_bridge_port **port,
>> +				 struct net_bridge_vlan **vlan)
>> +{
>> +	struct net_bridge_vlan *v;
>> +	unsigned long value;
>> +
>> +	value = ACCESS_PRIVATE(&dst, value);
>> +	if (!(value & BR_DST_VLAN_TAG)) {
>> +		*port = (struct net_bridge_port *)value;
>> +		*vlan = NULL;
>> +		return;
>> +	}
>> +
>> +	v = (struct net_bridge_vlan *)(value & ~BR_DST_VLAN_TAG);
>> +	*port = v->port;
> 
> [Severity: Low]
> This isn't a bug, but br_vlan_to_dst() takes any struct net_bridge_vlan
> pointer, and br_dst_decode() reads v->port without checking which kind of
> VLAN entry it has.
> 
> In struct net_bridge_vlan, port is in a union with br:
> 
> 	union {
> 		struct net_bridge	*br;
> 		struct net_bridge_port	*port;
> 	};
> 
> For a bridge master VLAN, br is the active member. Decoding one would hand
> back a struct net_bridge pointer typed as a struct net_bridge_port.
> Passing NULL would encode to 0x1, and decoding that would dereference a
> near-NULL address.
> 
> As far as I can tell, no caller can hit this. Later in the series, all of
> br_fdb_update(), __br_fdb_add(), br_fdb_resolve_dst() and
> nbp_vlan_delete() pass a non-NULL VLAN taken from a port VLAN group.
> __br_fdb_add() also excludes the bridge master case on purpose:
> 
> 	dst = p && vlan ? br_vlan_to_dst(vlan) : br_port_to_dst(p);
> 
> Would a comment on br_vlan_to_dst() be worth adding, stating that it only
> takes non-NULL port VLANs? Something like
> WARN_ON_ONCE(br_vlan_is_master(v)) would also enforce that.
> 

- No, that is clear from the code and its usage. It would be redundant.

>> +	*vlan = v;
>> +}
>> +
> 
> [ ... ]
>