Re: [PATCH net-next 08/12] net: bridge: vlan: quiesce readers before freeing port VLANs

[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 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] nbp_vlan_delete() in net/bridge/br_vlan.c now calls
>    synchronize_net() for every port-VLAN it deletes.
> - [Low] br_fdb_cleanup_by_dst() in net/bridge/br_fdb.c reads f->dst once
>    into `dst`.
> 
> --
> 

I already answered, but here it is again:

> net: bridge: vlan: quiesce readers before freeing port VLANs
> 
> This patch prepares for fdb entries caching port-VLAN pointers. A port
> VLAN is now unpublished, a grace period passes, and only then are the fdb
> references purged or rewritten and the VLAN freed. During port teardown
> the whole VLAN group is unpublished first, so all VLANs share one grace
> period.
> 
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index 7c68b540b3580..307f9c12914ef 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -883,7 +883,9 @@ void br_fdb_cleanup_by_dst(struct net_bridge *br,
>>
>>   	spin_lock_bh(&br->hash_lock);
>>   	hlist_for_each_entry_safe(f, tmp, &br->fdb_list, fdb_node) {
>> -		if (br_fdb_dst_port(f) != p)
>> +		struct net_bridge_dst dst = br_fdb_dst_read(f);
>> +
>> +		if (br_dst_port(dst) != p)
>>   			continue;
>>
> 
> [ ... ]
> 
>> +			     !test_bit(BR_FDB_OFFLOADED, &f->flags))) {
>> +				/* The entry outlives the VLAN, so it must fall
>> +				 * back to the raw port destination
>> +				 */
>> +				if (vlan && br_dst_vlan(dst) == vlan)
>> +					br_fdb_dst_write(f,
>> +							 br_port_to_dst(p));
> 
> [Severity: Low]
> Can this unconditional write lose a concurrent roam?
> 
> dst is read once at the top of the loop. br_fdb_update() roams entries by
> writing fdb->dst under RCU only, without br->hash_lock:
> 
> br_fdb_update()
>      br_fdb_dst_write(fdb, br_port_to_dst(source));
> 
> If a roam lands between br_fdb_dst_read() and this br_fdb_dst_write(), the
> new port would be overwritten with p.
> 
> This branch can't be reached at this commit. Every br_fdb_dst_write()
> caller stores br_port_to_dst(), so br_dst_vlan(dst) is always NULL here.
> 
> Later in the series, "net: bridge: fdb: cache port VLANs in learned
> entries" replaces this with br_fdb_dst_replace(f, dst, br_port_to_dst(p)).
> That is a cmpxchg against the snapshot, so a concurrent roam is kept.
> 
> Would it make sense to use the conditional replace here from the start?
> 

- No, we're fine with the roam race, it is normal. Trying to change these races
   requires much more work and isn't worth it at the moment.

>>   				continue;
>> +			}
>> +		}
> 
> [ ... ]
> 
>> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
>> index 8a914e249767a..0675f74cf1b99 100644
>> --- a/net/bridge/br_vlan.c
>> +++ b/net/bridge/br_vlan.c
>> @@ -1406,23 +1406,32 @@ int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
>>   	v = br_vlan_find(vg, vid);
>>   	if (!v)
>>   		return -ENOENT;
>> -	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
>>   	__vlan_unpublish(vg, v);
>> +	synchronize_net();
> 
> [Severity: Medium]
> Does this add one expedited grace period for each port VLAN deleted, all
> while RTNL is held?
> 
> Every caller of nbp_vlan_delete() holds RTNL, so synchronize_net() always
> goes through synchronize_rcu_expedited(). Range requests are processed one
> VID at a time under a single RTNL hold:
> 
> br_process_vlan_info()
>      for (v = (*vinfo_last)->vid; v <= vinfo_curr->vid; v++) {
>          br_vlan_info()
>              nbp_vlan_delete(p, vinfo->vid)
>                  synchronize_net()
> 
> So a request like "bridge vlan del dev swp1 vid 1-4094" could run up to
> 4094 expedited grace periods back to back with RTNL held. The
> cond_resched() in that loop doesn't drop RTNL.
> 
> The per-port loops in br_vlan_disable_default_pvid() and
> __br_vlan_set_default_pvid() also now take one grace period per bridge
> port. That includes the error unwind loop in __br_vlan_set_default_pvid().
> 
> Before this patch, nbp_vlan_delete() waited for no grace period at all,
> because __vlan_del() only queues call_rcu().
> 
> The commit message only describes batching for port teardown:
> 
>    "This lets all VLANs share one grace period."
> 
> Could range deletion be batched the same way? For example, unpublish the
> whole range, wait once, then clean the fdbs and free the VLANs.
> 
> At this commit, no fdb writer stores a VLAN dst yet. That starts with "net:
> bridge: fdb: cache port VLANs in learned entries". The per-VLAN
> synchronize_net() is still unchanged at the end of the series, in "net:
> bridge: fdb: avoid VLAN lookups in unicast forwarding".
> 

- Yes, they can be batched and I had them batched initially but when I ran my tests I didn't
   notice any problems deleting a full VLAN range. It took 79ms on my host to delete 4k VLANs.
   As I've stated in my cover letter - the complexity is not worth it, so I dropped that patch.

   (from my previous reply)

>> +	/* Traffic may still use v through cached fdb dsts until they are
>> +	 * cleaned below. This is acceptable during vlan deletion. Above we
>> +	 * drain the readers that could republish the dst before cleaning it
>> +	 */
>> +	br_fdb_cleanup_by_dst(port->br, br_vlan_to_dst(v), vid, 0);
>>   	__vlan_del(vg, v);
>>
>>   	return 0;
>>   }
> 
> [ ... ]
>