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

[Nikolay Aleksandrov <razor@blackwall.org>]

On 30/09/2026 10:14, Nikolay Aleksandrov wrote:
> Later fdb entries will cache port-VLAN pointers so unpublish a VLAN, wait
> for a grace period (existing readers) and then purge or rewrite fdb
> references before releasing it. Cached destinations can continue forwarding
> until they are cleaned, that is acceptable so add a comment to document it.
> During port teardown unpublish the complete VLAN group first, clean the
> port fdbs and then release the VLANs. This lets all VLANs share one grace
> period.
> 
> Reviewed-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: Nikolay Aleksandrov <razor@blackwall.org>
> ---
>   net/bridge/br_fdb.c     | 20 ++++++++++++++++----
>   net/bridge/br_if.c      |  7 +++++--
>   net/bridge/br_private.h | 12 ++++++++++--
>   net/bridge/br_vlan.c    | 21 +++++++++++++++------
>   4 files changed, 46 insertions(+), 14 deletions(-)
> 
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index 7c68b540b358..307f9c12914e 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;
>   
>   		if (vlan && f->key.vlan_id == vlan->vid &&
> @@ -894,12 +896,22 @@ void br_fdb_cleanup_by_dst(struct net_bridge *br,
>   			continue;
>   		}
>   
> -		if (!do_all)
> +		if (!do_all) {
> +			if (vid && f->key.vlan_id != vid)
> +				continue;
> +
>   			if (test_bit(BR_FDB_STATIC, &f->flags) ||
>   			    (test_bit(BR_FDB_ADDED_BY_EXT_LEARN, &f->flags) &&
> -			     !test_bit(BR_FDB_OFFLOADED, &f->flags)) ||
> -			    (vid && f->key.vlan_id != vid))
> +			     !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));
>   				continue;
> +			}
> +		}
>   
>   		if (test_bit(BR_FDB_LOCAL, &f->flags))
>   			fdb_delete_local(br, p, f);
> diff --git a/net/bridge/br_if.c b/net/bridge/br_if.c
> index d94558a5e3e9..2c05ebc1299d 100644
> --- a/net/bridge/br_if.c
> +++ b/net/bridge/br_if.c
> @@ -333,8 +333,9 @@ static void update_headroom(struct net_bridge *br, int new_hr)
>    */
>   static void del_nbp(struct net_bridge_port *p)
>   {
> -	struct net_bridge *br = p->br;
> +	struct net_bridge_vlan_group *vg;
>   	struct net_device *dev = p->dev;
> +	struct net_bridge *br = p->br;
>   
>   	sysfs_remove_link(br->ifobj, p->dev->name);
>   
> @@ -354,8 +355,10 @@ static void del_nbp(struct net_bridge_port *p)
>   		update_headroom(br, get_max_headroom(br));
>   	netdev_reset_rx_headroom(dev);
>   
> -	nbp_vlan_flush(p);
> +	vg = nbp_vlan_group(p);
> +	nbp_vlan_group_unpublish(p);
>   	br_fdb_cleanup_by_dst(br, br_port_to_dst(p), 0, 1);
> +	nbp_vlan_flush(p, vg);
>   	switchdev_deferred_process();
>   	nbp_backup_clear(p);
>   
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index a790368b69e9..951b6ac5f484 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -1735,7 +1735,9 @@ int __br_vlan_set_default_pvid(struct net_bridge *br, u16 pvid,
>   int nbp_vlan_add(struct net_bridge_port *port, u16 vid, u16 flags,
>   		 bool *changed, struct netlink_ext_ack *extack);
>   int nbp_vlan_delete(struct net_bridge_port *port, u16 vid);
> -void nbp_vlan_flush(struct net_bridge_port *port);
> +void nbp_vlan_group_unpublish(struct net_bridge_port *port);
> +void nbp_vlan_flush(struct net_bridge_port *port,
> +		    struct net_bridge_vlan_group *vg);
>   int nbp_vlan_init(struct net_bridge_port *port, struct netlink_ext_ack *extack);
>   int nbp_get_num_vlan_infos(struct net_bridge_port *p, u32 filter_mask);
>   void br_vlan_get_stats(const struct net_bridge_vlan *v,
> @@ -1894,7 +1896,13 @@ static inline int nbp_vlan_delete(struct net_bridge_port *port, u16 vid)
>   	return -EOPNOTSUPP;
>   }
>   
> -static inline void nbp_vlan_flush(struct net_bridge_port *port)
> +static inline void nbp_vlan_group_unpublish(struct net_bridge_port *port)
> +{
> +}
> +
> +static inline void
> +nbp_vlan_flush(struct net_bridge_port *port,
> +	       struct net_bridge_vlan_group *vg)
>   {
>   }
>   
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 8a914e249767..0675f74cf1b9 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();

Sashiko says:
  Does this synchronize_net() call introduce an RTNL stall and IPI storm when
  processing a large VLAN range?
  When a user issues an RTM_DELLINK command specifying a large range of VLANs
  (e.g., vid 1-4094), br_process_vlan_info() iterates over the range sequentially:
  net/bridge/br_netlink.c:br_process_vlan_info() {
      ...
  		for (v = (*vinfo_last)->vid; v <= vinfo_curr->vid; v++) {
  			bool curr_change = false;
  			tmp_vinfo.vid = v;
  			err = br_vlan_info(br, p, cmd, &tmp_vinfo, &curr_change,
  					   extack);
      ...
  }
  This loop calls into br_vlan_info(), which then calls nbp_vlan_delete() for
  each VLAN. Because the global RTNL lock is held during this process,
  synchronize_net() translates to synchronize_rcu_expedited().
  Calling this iteratively up to 4094 times blocks the RTNL lock for several
  seconds and spams all CPUs with IPIs, causing a severe system-wide network
  configuration Denial of Service. This is reachable by any user with
  CAP_NET_ADMIN in the network namespace, which can often be obtained by
  unprivileged local users via user namespaces.
  Could these grace periods be batched or deferred when deleting multiple VLANs?

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

> +	/* 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;
>   }
>   
> -void nbp_vlan_flush(struct net_bridge_port *port)
> +void nbp_vlan_group_unpublish(struct net_bridge_port *port)
>   {
> -	struct net_bridge_vlan_group *vg;
> -
>   	ASSERT_RTNL();
>   
> -	vg = nbp_vlan_group(port);
> -	__vlan_flush(port->br, port, vg);
>   	RCU_INIT_POINTER(port->vlgrp, NULL);
>   	synchronize_net();
> +}
> +
> +void nbp_vlan_flush(struct net_bridge_port *port,
> +		    struct net_bridge_vlan_group *vg)
> +{
> +	ASSERT_RTNL();
> +
> +	__vlan_flush(port->br, port, vg);
>   	__vlan_group_free(vg);
>   }
>