Re: [PATCH net-next v2 1/9] net: bridge: vlan: drop legacy memory barriers

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (1)

- [Medium] Writer-side publish ordering removed: in `__vlan_add()`
  (net/bridge/br_vlan.c) a new VLAN is made visible to RCU readers…

Pre-existing issues:
- [Low] Concern (retained for policy combination, but validated as
  resolved within this series): after this patch `vg->pvid` is read and…

--

net: bridge: vlan: drop legacy memory barriers

This removes the smp_wmb() calls from __vlan_add_pvid() and
__vlan_delete_pvid() and the smp_rmb() from br_get_pvid(), on the grounds
that the pvid is written under lock and read under rcu in the fast-path.
A couple of questions about the write side below.

> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index d337b1cfb980..5b2c22ad669e 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -1687,7 +1687,6 @@ static inline u16 br_get_pvid(const struct net_bridge_vlan_group *vg)
>  	if (!vg)
>  		return 0;
>  
> -	smp_rmb();
>  	return vg->pvid;
>  }

[Severity: Low]
This isn't a bug introduced by this patch, but with the barriers gone
vg->pvid is left as a plain C access on both sides: br_get_pvid() does
"return vg->pvid;", and the writers in br_vlan.c do "vg->pvid = v->vid;"
and "vg->pvid = 0;". There is also a direct read outside the helper in
br_mst_vlan_set_state():

net/bridge/br_mst.c:br_mst_vlan_set_state() {
	...
	if (v->vid == vg->pvid)
	...
}

The sibling field in the same struct is only touched through accessors
that mark the access:

net/bridge/br_private.h {
	static inline void br_vlan_set_pvid_state(struct net_bridge_vlan_group *vg,
						  u8 state)
	{
		WRITE_ONCE(vg->pvid_state, state);
	}
}

The follow-up patches in this series do address this - "net: bridge: vlan:
annotate lockless pvid use" converts br_get_pvid() to READ_ONCE() and both
writers to WRITE_ONCE(), and "net: bridge: mst: use br_get_pvid helper"
converts the br_mst.c read - so this only applies to the intermediate tree
state at this commit. Would it be simpler to fold the annotation into this
patch so no intermediate state has an unmarked pvid access?

> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 1e0e436629ec..2ae6ffb23399 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -40,7 +40,6 @@ static void __vlan_add_pvid(struct net_bridge_vlan_group *vg,
>  	if (vg->pvid == v->vid)
>  		return;
>  
> -	smp_wmb();
>  	br_vlan_set_pvid_state(vg, v->state);
>  	vg->pvid = v->vid;
>  }

[Severity: Medium]
The commit message says "nothing to commit prior to changing it".  Is that
accurate for the add path?  In __vlan_add() the vlan is published to rcu
readers first, and the pvid store happens after that:

net/bridge/br_vlan.c:__vlan_add() {
	...
	/* set the state before publishing */
	br_vlan_init_state(v);

	err = rhashtable_lookup_insert_fast(&vg->vlan_hash, &v->vnode,
					    br_vlan_rht_params);
	if (err)
		goto out_fdb_insert;

	__vlan_add_list(v);
	__vlan_flags_commit(v, flags);	/* -> __vlan_add_pvid() */
	...
}

So the removed smp_wmb() sat between the rhashtable insert / list add and
the vg->pvid store.  On a weakly ordered architecture, can a fast-path
reader observe the new vg->pvid before the matching net_bridge_vlan is
visible in vg->vlan_hash?  The rhashtable bucket store has release
semantics for the insert itself, but nothing orders the later plain pvid
store after it, and the WRITE_ONCE() added by the next patch in the series
does not provide store-store ordering either.

If that can happen, the reader in __allowed_ingress() picks up the new pvid
and then fails the lookup:

net/bridge/br_vlan.c:__allowed_ingress() {
	...
	v = br_vlan_find(vg, *vid);
	if (!v || !br_vlan_should_use(v))
		goto drop;
	...
}

which would drop untagged/priority-tagged frames for the length of the
window (this lookup is reached when BROPT_MCAST_VLAN_SNOOPING_ENABLED or
BROPT_VLAN_STATS_ENABLED is set), triggered by something like
"bridge vlan add dev <port> vid N pvid untagged" for a new vlan while
untagged traffic is received on that port.

For the read side the analysis looks fine: the smp_rmb() in br_get_pvid()
preceded the pvid load, so it never ordered the pvid load against the
following hash lookup - that direction comes from the address dependency
into the bucket computation.

If the write-side ordering is still wanted without a fence in the
fast-path, would smp_store_release(&vg->pvid, v->vid) here be the right
replacement?

> @@ -50,7 +49,6 @@ static void __vlan_delete_pvid(struct net_bridge_vlan_group *vg, u16 vid)
>  	if (vg->pvid != vid)
>  		return;
>  
> -	smp_wmb();
>  	vg->pvid = 0;
>  }

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