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

[netdev-bot+sashiko@kernel.org]

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`.

--

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?

>  				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".

> +	/* 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;
>  }

[ ... ]

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