Re: [PATCH net-next v3 03/14] net: bridge: mcast: avoid sleeping on bridge-down

[Nikolay Aleksandrov <razor@blackwall.org>]

On Mon, Mar 02, 2026 at 06:39:57AM +0100, Linus Lüssing wrote:
> We later want to use the multicast lock when setting the bridge
> interface up or down, to be able to atomically both check all conditions
> to toggle the multicast active state and to subsequently toggle it.
> While most variables we check / contexts we check from are serialized
> (toggled variables through netlink/sysfs) the timer_pending() check is
> not and might run in parallel.
> 
> However so far we are not allowed to spinlock __br_multicast_stop() as
> its call to timer_delete_sync() might sleep. Therefore replacing the
> sleeping variant with the non-sleeping timer_shutdown(). It is sufficient
> to only wait for any timer callback to finish when we are freeing the
> multicast context.
> 
> While the disadvantage of using a non-syncing variant might lead us to
> race and still execute its timer callback just after timer_shutdown() was
> called timer_shutdown() also has the following additional advantage(s):
> It for one thing clears the callback function pointer and by that avoids
> rearming. For another a missing function pointer allows us to detect
> early in the timer callback if we, this timer, were just canceled.
> 
> In other words, this also allows us to make sure that once
> timer_shutdown() was called while we do potentially enter its timer
> callback briefly we never run the main task of this timer. Similar to
> what a timer_delete_sync() would have avoided, too. Except we are not
> waiting/sleeping/syncing in br_multicast_stop() but instead (in rare cases)
> briefly busy-wait/sync a bit later when grabbing the multicast spinlock
> in the timer callback.
> 
> This new check also makes the netif_running() check redundant/obsolete
> in these contexts.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---
>  net/bridge/br_multicast.c | 128 +++++++++++++++++++++++++++-----------
>  net/bridge/br_private.h   |   5 ++
>  net/bridge/br_vlan.c      |   5 ++
>  3 files changed, 100 insertions(+), 38 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index 881d866d687a..b90f0e149c40 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1663,6 +1663,14 @@ static void br_multicast_router_expired(struct net_bridge_mcast_port *pmctx,
>  	spin_unlock(&br->multicast_lock);
>  }
>  
> +static bool br_multicast_is_stopping(struct net_bridge *br,
> +				     struct timer_list *timer)

both should be const

> +{
> +	lockdep_assert_held_once(&br->multicast_lock);
> +
> +	return !timer->function;
> +}
> +
>  static void br_ip4_multicast_router_expired(struct timer_list *t)
>  {
>  	struct net_bridge_mcast_port *pmctx = timer_container_of(pmctx, t,
> @@ -1698,7 +1706,8 @@ static void br_multicast_local_router_expired(struct net_bridge_mcast *brmctx,
>  					      struct timer_list *timer)
>  {
>  	spin_lock(&brmctx->br->multicast_lock);
> -	if (brmctx->multicast_router == MDB_RTR_TYPE_DISABLED ||
> +	if (br_multicast_is_stopping(brmctx->br, timer) ||
> +	    brmctx->multicast_router == MDB_RTR_TYPE_DISABLED ||
>  	    brmctx->multicast_router == MDB_RTR_TYPE_PERM ||
>  	    br_ip4_multicast_is_router(brmctx) ||
>  	    br_ip6_multicast_is_router(brmctx))
> @@ -1728,10 +1737,11 @@ static void br_ip6_multicast_local_router_expired(struct timer_list *t)
>  #endif
>  
>  static void br_multicast_querier_expired(struct net_bridge_mcast *brmctx,
> -					 struct bridge_mcast_own_query *query)
> +					 struct bridge_mcast_own_query *query,
> +					 struct timer_list *timer)
>  {
>  	spin_lock(&brmctx->br->multicast_lock);
> -	if (!netif_running(brmctx->br->dev) ||
> +	if (br_multicast_is_stopping(brmctx->br, timer) ||
>  	    br_multicast_ctx_vlan_global_disabled(brmctx) ||
>  	    !br_opt_get(brmctx->br, BROPT_MULTICAST_ENABLED))
>  		goto out;
> @@ -1747,7 +1757,7 @@ static void br_ip4_multicast_querier_expired(struct timer_list *t)
>  	struct net_bridge_mcast *brmctx = timer_container_of(brmctx, t,
>  							     ip4_other_query.timer);
>  
> -	br_multicast_querier_expired(brmctx, &brmctx->ip4_own_query);
> +	br_multicast_querier_expired(brmctx, &brmctx->ip4_own_query, t);
>  }
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> @@ -1756,7 +1766,7 @@ static void br_ip6_multicast_querier_expired(struct timer_list *t)
>  	struct net_bridge_mcast *brmctx = timer_container_of(brmctx, t,
>  							     ip6_other_query.timer);
>  
> -	br_multicast_querier_expired(brmctx, &brmctx->ip6_own_query);
> +	br_multicast_querier_expired(brmctx, &brmctx->ip6_own_query, t);
>  }
>  #endif
>  
> @@ -4040,10 +4050,12 @@ int br_multicast_rcv(struct net_bridge_mcast **brmctx,
>  }
>  
>  static void br_multicast_query_expired(struct net_bridge_mcast *brmctx,
> -				       struct bridge_mcast_own_query *query)
> +				       struct bridge_mcast_own_query *query,
> +				       struct timer_list *timer)
>  {
>  	spin_lock(&brmctx->br->multicast_lock);
> -	if (br_multicast_ctx_vlan_disabled(brmctx))
> +	if (br_multicast_is_stopping(brmctx->br, timer) ||
> +	    br_multicast_ctx_vlan_disabled(brmctx))
>  		goto out;
>  
>  	if (query->startup_sent < brmctx->multicast_startup_query_count)
> @@ -4059,7 +4071,7 @@ static void br_ip4_multicast_query_expired(struct timer_list *t)
>  	struct net_bridge_mcast *brmctx = timer_container_of(brmctx, t,
>  							     ip4_own_query.timer);
>  
> -	br_multicast_query_expired(brmctx, &brmctx->ip4_own_query);
> +	br_multicast_query_expired(brmctx, &brmctx->ip4_own_query, t);
>  }
>  
>  #if IS_ENABLED(CONFIG_IPV6)
> @@ -4068,7 +4080,7 @@ static void br_ip6_multicast_query_expired(struct timer_list *t)
>  	struct net_bridge_mcast *brmctx = timer_container_of(brmctx, t,
>  							     ip6_own_query.timer);
>  
> -	br_multicast_query_expired(brmctx, &brmctx->ip6_own_query);
> +	br_multicast_query_expired(brmctx, &brmctx->ip6_own_query, t);
>  }
>  #endif
>  
> @@ -4111,29 +4123,30 @@ void br_multicast_ctx_init(struct net_bridge *br,
>  	seqcount_spinlock_init(&brmctx->ip6_querier.seq, &br->multicast_lock);
>  #endif
>  
> -	timer_setup(&brmctx->ip4_mc_router_timer,
> -		    br_ip4_multicast_local_router_expired, 0);
> -	timer_setup(&brmctx->ip4_other_query.timer,
> -		    br_ip4_multicast_querier_expired, 0);
> -	timer_setup(&brmctx->ip4_other_query.delay_timer,
> -		    br_multicast_query_delay_expired, 0);
> -	timer_setup(&brmctx->ip4_own_query.timer,
> -		    br_ip4_multicast_query_expired, 0);
> +	timer_setup(&brmctx->ip4_mc_router_timer, NULL, 0);
> +	timer_setup(&brmctx->ip4_other_query.timer, NULL, 0);
> +	timer_setup(&brmctx->ip4_other_query.delay_timer, NULL, 0);
> +	timer_setup(&brmctx->ip4_own_query.timer, NULL, 0);
>  #if IS_ENABLED(CONFIG_IPV6)
> -	timer_setup(&brmctx->ip6_mc_router_timer,
> -		    br_ip6_multicast_local_router_expired, 0);
> -	timer_setup(&brmctx->ip6_other_query.timer,
> -		    br_ip6_multicast_querier_expired, 0);
> -	timer_setup(&brmctx->ip6_other_query.delay_timer,
> -		    br_multicast_query_delay_expired, 0);
> -	timer_setup(&brmctx->ip6_own_query.timer,
> -		    br_ip6_multicast_query_expired, 0);
> +	timer_setup(&brmctx->ip6_mc_router_timer, NULL, 0);
> +	timer_setup(&brmctx->ip6_other_query.timer, NULL, 0);
> +	timer_setup(&brmctx->ip6_other_query.delay_timer, NULL, 0);
> +	timer_setup(&brmctx->ip6_own_query.timer, NULL, 0);
>  #endif
>  }
>  
>  void br_multicast_ctx_deinit(struct net_bridge_mcast *brmctx)
>  {
> -	__br_multicast_stop(brmctx);
> +	timer_shutdown_sync(&brmctx->ip4_mc_router_timer);
> +	timer_shutdown_sync(&brmctx->ip4_other_query.timer);
> +	timer_shutdown_sync(&brmctx->ip4_other_query.delay_timer);
> +	timer_shutdown_sync(&brmctx->ip4_own_query.timer);
> +#if IS_ENABLED(CONFIG_IPV6)
> +	timer_shutdown_sync(&brmctx->ip6_mc_router_timer);
> +	timer_shutdown_sync(&brmctx->ip6_other_query.timer);
> +	timer_shutdown_sync(&brmctx->ip6_other_query.delay_timer);
> +	timer_shutdown_sync(&brmctx->ip6_own_query.timer);
> +#endif
>  }
>  
>  void br_multicast_init(struct net_bridge *br)
> @@ -4213,9 +4226,27 @@ void br_multicast_leave_snoopers(struct net_bridge *br)
>  	br_ip6_multicast_leave_snoopers(br);
>  }
>  
> +void br_multicast_reset_timer_cbs(struct net_bridge_mcast *brmctx)
> +{
> +	lockdep_assert_held_once(&brmctx->br->multicast_lock);
> +
> +	brmctx->ip4_mc_router_timer.function = br_ip4_multicast_local_router_expired;
> +	brmctx->ip4_other_query.timer.function = br_ip4_multicast_querier_expired;
> +	brmctx->ip4_other_query.delay_timer.function = br_multicast_query_delay_expired;
> +	brmctx->ip4_own_query.timer.function = br_ip4_multicast_query_expired;
> +#if IS_ENABLED(CONFIG_IPV6)
> +	brmctx->ip6_mc_router_timer.function = br_ip6_multicast_local_router_expired;
> +	brmctx->ip6_other_query.timer.function = br_ip6_multicast_querier_expired;
> +	brmctx->ip6_other_query.delay_timer.function = br_multicast_query_delay_expired;
> +	brmctx->ip6_own_query.timer.function = br_ip6_multicast_query_expired;
> +#endif
> +}
> +
>  static void __br_multicast_open_query(struct net_bridge *br,
>  				      struct bridge_mcast_own_query *query)
>  {
> +	lockdep_assert_held_once(&br->multicast_lock);
> +
>  	query->startup_sent = 0;
>  
>  	if (!br_opt_get(br, BROPT_MULTICAST_ENABLED))
> @@ -4226,13 +4257,15 @@ static void __br_multicast_open_query(struct net_bridge *br,
>  
>  static void __br_multicast_open(struct net_bridge_mcast *brmctx)
>  {
> +	br_multicast_reset_timer_cbs(brmctx);
> +
>  	__br_multicast_open_query(brmctx->br, &brmctx->ip4_own_query);
>  #if IS_ENABLED(CONFIG_IPV6)
>  	__br_multicast_open_query(brmctx->br, &brmctx->ip6_own_query);
>  #endif
>  }
>  
> -void br_multicast_open(struct net_bridge *br)
> +static void br_multicast_open_locked(struct net_bridge *br)
>  {
>  	ASSERT_RTNL();
>  
> @@ -4256,17 +4289,26 @@ void br_multicast_open(struct net_bridge *br)
>  	}
>  }
>  
> +void br_multicast_open(struct net_bridge *br)
> +{
> +	spin_lock_bh(&br->multicast_lock);
> +	br_multicast_open_locked(br);
> +	spin_unlock_bh(&br->multicast_lock);
> +}
> +
>  static void __br_multicast_stop(struct net_bridge_mcast *brmctx)
>  {
> -	timer_delete_sync(&brmctx->ip4_mc_router_timer);
> -	timer_delete_sync(&brmctx->ip4_other_query.timer);
> -	timer_delete_sync(&brmctx->ip4_other_query.delay_timer);
> -	timer_delete_sync(&brmctx->ip4_own_query.timer);
> +	lockdep_assert_held_once(&brmctx->br->multicast_lock);
> +
> +	timer_shutdown(&brmctx->ip4_mc_router_timer);
> +	timer_shutdown(&brmctx->ip4_other_query.timer);
> +	timer_shutdown(&brmctx->ip4_other_query.delay_timer);
> +	timer_shutdown(&brmctx->ip4_own_query.timer);
>  #if IS_ENABLED(CONFIG_IPV6)
> -	timer_delete_sync(&brmctx->ip6_mc_router_timer);
> -	timer_delete_sync(&brmctx->ip6_other_query.timer);
> -	timer_delete_sync(&brmctx->ip6_other_query.delay_timer);
> -	timer_delete_sync(&brmctx->ip6_own_query.timer);
> +	timer_shutdown(&brmctx->ip6_mc_router_timer);
> +	timer_shutdown(&brmctx->ip6_other_query.timer);
> +	timer_shutdown(&brmctx->ip6_other_query.delay_timer);
> +	timer_shutdown(&brmctx->ip6_own_query.timer);
>  #endif
>  }
>  
> @@ -4317,12 +4359,12 @@ void br_multicast_toggle_one_vlan(struct net_bridge_vlan *vlan, bool on)
>  
>  		spin_lock_bh(&br->multicast_lock);
>  		vlan->priv_flags ^= BR_VLFLAG_MCAST_ENABLED;
> -		spin_unlock_bh(&br->multicast_lock);
>  
>  		if (on)
>  			__br_multicast_open(&vlan->br_mcast_ctx);
>  		else
>  			__br_multicast_stop(&vlan->br_mcast_ctx);
> +		spin_unlock_bh(&br->multicast_lock);
>  	} else {
>  		struct net_bridge_mcast *brmctx;
>  
> @@ -4380,6 +4422,7 @@ int br_multicast_toggle_vlan_snooping(struct net_bridge *br, bool on,
>  	if (!vg)
>  		return 0;
>  
> +	spin_lock_bh(&br->multicast_lock);
>  	br_opt_toggle(br, BROPT_MCAST_VLAN_SNOOPING_ENABLED, on);
>  
>  	/* disable/enable non-vlan mcast contexts based on vlan snooping */
> @@ -4387,6 +4430,8 @@ int br_multicast_toggle_vlan_snooping(struct net_bridge *br, bool on,
>  		__br_multicast_stop(&br->multicast_ctx);
>  	else
>  		__br_multicast_open(&br->multicast_ctx);
> +	spin_unlock_bh(&br->multicast_lock);
> +
>  	list_for_each_entry(p, &br->port_list, list) {
>  		if (on)
>  			br_multicast_disable_port_ctx(&p->multicast_ctx);
> @@ -4416,7 +4461,7 @@ bool br_multicast_toggle_global_vlan(struct net_bridge_vlan *vlan, bool on)
>  	return true;
>  }
>  
> -void br_multicast_stop(struct net_bridge *br)
> +static void br_multicast_stop_locked(struct net_bridge *br)
>  {
>  	ASSERT_RTNL();
>  
> @@ -4440,6 +4485,13 @@ void br_multicast_stop(struct net_bridge *br)
>  	}
>  }
>  
> +void br_multicast_stop(struct net_bridge *br)
> +{
> +	spin_lock_bh(&br->multicast_lock);
> +	br_multicast_stop_locked(br);
> +	spin_unlock_bh(&br->multicast_lock);
> +}
> +
>  void br_multicast_dev_del(struct net_bridge *br)
>  {
>  	struct net_bridge_mdb_entry *mp;
> @@ -4675,7 +4727,7 @@ int br_multicast_toggle(struct net_bridge *br, unsigned long val,
>  	if (!netif_running(br->dev))
>  		goto unlock;
>  
> -	br_multicast_open(br);
> +	br_multicast_open_locked(br);
>  	list_for_each_entry(port, &br->port_list, list)
>  		__br_multicast_enable_port_ctx(&port->multicast_ctx);
>  
> diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
> index 4ab6a1f58116..a181a27aa559 100644
> --- a/net/bridge/br_private.h
> +++ b/net/bridge/br_private.h
> @@ -976,6 +976,7 @@ void br_multicast_disable_port(struct net_bridge_port *port);
>  void br_multicast_init(struct net_bridge *br);
>  void br_multicast_join_snoopers(struct net_bridge *br);
>  void br_multicast_leave_snoopers(struct net_bridge *br);
> +void br_multicast_reset_timer_cbs(struct net_bridge_mcast *brmctx);
>  void br_multicast_open(struct net_bridge *br);
>  void br_multicast_stop(struct net_bridge *br);
>  void br_multicast_dev_del(struct net_bridge *br);
> @@ -1416,6 +1417,10 @@ static inline void br_multicast_leave_snoopers(struct net_bridge *br)
>  {
>  }
>  
> +static inline void br_multicast_reset_timer_cbs(struct net_bridge_mcast *brmctx)
> +{
> +}
> +
>  static inline void br_multicast_open(struct net_bridge *br)
>  {
>  }
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 326933b455b3..3facb4eda306 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -325,7 +325,12 @@ static int __vlan_add(struct net_bridge_vlan *v, u16 flags,
>  			if (err && err != -EOPNOTSUPP)
>  				goto out;
>  		}
> +

extra new line

>  		br_multicast_ctx_init(br, v, &v->br_mcast_ctx);
> +
> +		spin_lock_bh(&br->multicast_lock);
> +		br_multicast_reset_timer_cbs(&v->br_mcast_ctx);
> +		spin_unlock_bh(&br->multicast_lock);

Have you tested this without bridge IGMP_SNOOPING defined?
I don't think it will compile.

Also, please avoid spilling multicast lock outside of mcast code. In fact
why don't you move this in br_multicast_ctx_init?

>  		v->priv_flags |= BR_VLFLAG_GLOBAL_MCAST_ENABLED;
>  	}
>  
> -- 
> 2.51.0
>