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

[Nikolay Aleksandrov <razor@blackwall.org>]

On 05/09/2026 23:27, netdev-bot+sashiko@kernel.org wrote:
> 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…
> 

I don't think there's anything wrong with the patch, I've answered below.

> --
> 
> 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?
> 

This is not the point of the change here and it isn't relevant at all.

>> 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?
> 

This synchronization window is acceptable, we might drop a few packets. The write barrier
here keeping it shorter was really an accident, the barriers predate my conversion of
the vlan code to rhashtable when bitmaps were used and needed the sync.