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 <[email protected]>
> ---
> 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
>
