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

[netdev-bot+sashiko@kernel.org]

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.

> +	*vlan = v;
> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930071411.2786201-1-razor%40blackwall.org