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

On Fri, Feb 06, 2026 at 03:52:09AM +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 one. It is sufficient to only
> wait for any timer callback to finish when we are freeing the multicast
> context.
> 
> Using the timer_shutdown() instead of the timer_delete() variant also
> allows us to detect that we are stopping from within the according timer
> callbacks, to retain the promise of the previous timer_delete_sync()
> calls that no multicast state is changed after these
> timer_{delete,shutdown}*() calls. And more importantly that we are not
> inadvertently rearming timers.

Can you clarify what you mean by "allows us to detect that we are
stopping from within the according timer callbacks"?

> 
> This new check also makes the netif_running() check redundant/obsolete
> in these contexts.
> 
> Signed-off-by: Linus Lüssing <[email protected]>
> ---
>  net/bridge/br_device.c    |   4 ++
>  net/bridge/br_multicast.c | 108 ++++++++++++++++++++++++++------------
>  net/bridge/br_private.h   |   5 ++
>  net/bridge/br_vlan.c      |   5 ++
>  4 files changed, 87 insertions(+), 35 deletions(-)
> 
> diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c
> index a818fdc22da9..d9d1227d5708 100644
> --- a/net/bridge/br_device.c
> +++ b/net/bridge/br_device.c
> @@ -168,7 +168,9 @@ static int br_dev_open(struct net_device *dev)
>  	netdev_update_features(dev);
>  	netif_start_queue(dev);
>  	br_stp_enable_bridge(br);
> +	spin_lock_bh(&br->multicast_lock);
>  	br_multicast_open(br);

Maybe move the spin_lock_bh() / spin_unlock_bh() to br_multicast_open()
and have it call br_multicast_open_locked() that will also be invoked
from br_multicast_toggle()?

> +	spin_unlock_bh(&br->multicast_lock);
>  
>  	if (br_opt_get(br, BROPT_MULTICAST_ENABLED))
>  		br_multicast_join_snoopers(br);
> @@ -191,7 +193,9 @@ static int br_dev_stop(struct net_device *dev)
>  	struct net_bridge *br = netdev_priv(dev);
>  
>  	br_stp_disable_bridge(br);
> +	spin_lock_bh(&br->multicast_lock);
>  	br_multicast_stop(br);

And like br_multicast_open(), move the locking into br_multicast_stop()?

> +	spin_unlock_bh(&br->multicast_lock);
>  
>  	if (br_opt_get(br, BROPT_MULTICAST_ENABLED))
>  		br_multicast_leave_snoopers(br);
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index dccae08b4f4c..f5a368dd20a3 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1665,6 +1665,14 @@ static void br_multicast_router_expired(struct net_bridge_mcast_port *pmctx,
>  	spin_unlock(&br->multicast_lock);
>  }
>  
> +static bool br_multicast_stopping(struct net_bridge *br,

Nit: br_multicast_is_stopping() ?

> +				  struct timer_list *timer)
> +{
> +	lockdep_assert_held_once(&br->multicast_lock);
> +
> +	return !timer->function;
> +}