Re: [PATCH net-next v4 11/14] net: bridge: mcast: track active state, prepare for outside lock reads

[Ido Schimmel <idosch@nvidia.com>]

On Sat, Mar 07, 2026 at 05:45:45AM +0100, Linus Lüssing wrote:
> We are updating ip{4,6}_active and check all variables their state relies
> on while holding the bridge multicast spinlock. However we are going to
> read it without this lock in a follow-up commit on fast/data path, too.
> 
> As these variables are only booleans this shouldn't be a problem,
> ip{4,6}_active will be loaded and stored atomically. And those
> read sides should converge eventually. But to allow tooling to verify
> this use the READ_ONCE() and WRITE_ONCE macros.
> 
> Signed-off-by: Linus Lüssing <linus.luessing@c0d3.blue>
> ---
>  net/bridge/br_multicast.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
> index cb78f9555db6..6faa484dede7 100644
> --- a/net/bridge/br_multicast.c
> +++ b/net/bridge/br_multicast.c
> @@ -1085,9 +1085,10 @@ static void br_ip4_multicast_update_active(struct net_bridge_mcast *brmctx,
>  					   bool force_inactive)
>  {
>  	if (force_inactive)
> -		brmctx->ip4_active = false;
> +		WRITE_ONCE(brmctx->ip4_active, false);
>  	else
> -		brmctx->ip4_active = br_ip4_multicast_querier_exists(brmctx);
> +		WRITE_ONCE(brmctx->ip4_active,
> +			   br_ip4_multicast_querier_exists(brmctx));
>  }
>  
>  static void br_ip6_multicast_update_active(struct net_bridge_mcast *brmctx,
> @@ -1095,23 +1096,26 @@ static void br_ip6_multicast_update_active(struct net_bridge_mcast *brmctx,
>  {
>  #if IS_ENABLED(CONFIG_IPV6)
>  	if (force_inactive)
> -		brmctx->ip6_active = false;
> +		WRITE_ONCE(brmctx->ip6_active, false);
>  	else
> -		brmctx->ip6_active = br_ip6_multicast_querier_exists(brmctx);
> +		WRITE_ONCE(brmctx->ip6_active,
> +			   br_ip6_multicast_querier_exists(brmctx));
>  #endif
>  }
>  
>  static void br_multicast_notify_active(struct net_bridge_mcast *brmctx,
>  				       bool ip4_active_old, bool ip6_active_old)
>  {
> -	if (brmctx->ip4_active == ip4_active_old &&
> -	    brmctx->ip6_active == ip6_active_old)
> +	int ip4_active = brmctx->ip4_active;
> +	int ip6_active = brmctx->ip6_active;
> +
> +	if (ip4_active == ip4_active_old &&
> +	    ip6_active == ip6_active_old)
>  		return;
>  
>  	br_debug(brmctx->br, "mc_active changed, vid: %i: v4: %i->%i, v6: %i->%i\n",
>  		 brmctx->vlan ? brmctx->vlan->vid : -1,
> -		 ip4_active_old, brmctx->ip4_active,
> -		 ip6_active_old, brmctx->ip6_active);
> +		 ip4_active_old, ip4_active, ip6_active_old, ip6_active);
>  }

This hunk seems unnecessary? Pretty sure you can leave this function
as-is.

>  
>  /**
> @@ -4269,7 +4273,7 @@ void br_multicast_ctx_init(struct net_bridge *br,
>  	brmctx->multicast_membership_interval = 260 * HZ;
>  
>  	brmctx->ip4_querier.port_ifidx = 0;
> -	brmctx->ip4_active = 0;
> +	WRITE_ONCE(brmctx->ip4_active, 0);
>  	seqcount_spinlock_init(&brmctx->ip4_querier.seq, &br->multicast_lock);
>  	brmctx->multicast_igmp_version = 2;
>  #if IS_ENABLED(CONFIG_IPV6)
> @@ -4277,7 +4281,7 @@ void br_multicast_ctx_init(struct net_bridge *br,
>  	brmctx->ip6_querier.port_ifidx = 0;
>  	seqcount_spinlock_init(&brmctx->ip6_querier.seq, &br->multicast_lock);
>  #endif
> -	brmctx->ip6_active = 0;
> +	WRITE_ONCE(brmctx->ip6_active, 0);

Is this necessary? Who can read the active states while the multicast
context is still be initialized?

>  
>  	timer_setup(&brmctx->ip4_mc_router_timer, NULL, 0);
>  	timer_setup(&brmctx->ip4_other_query.timer, NULL, 0);
> -- 
> 2.53.0
>