Re: [PATCH net-next 10/14] netfilter: bridge: annotate data-races around br_netfilter and nf_log sysctls

[Simon Horman <horms@kernel.org>]

On Tue, Nov 10, 2026 at 05:53:24AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] brnf_sysctl_call_tables() in net/bridge/br_netfilter_hooks.c is
>   only half annotated.
> 
> --
> 
> netfilter: bridge: annotate data-races around br_netfilter and nf_log sysctls
> 
> This patch adds READ_ONCE() annotations in the packet path for the
> bridge-nf-* sysctls and for sysctl_nf_log_all_netns. It also adds a
> WRITE_ONCE() to the normalization store in brnf_sysctl_call_tables().
> The goal is to silence KCSAN data-race reports that show up when these
> sysctls change while packets are being bridged.
> 
> > diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
> > index e15355ff215a7..8a47fe4e8eb61 100644
> > --- a/net/bridge/br_netfilter_hooks.c
> > +++ b/net/bridge/br_netfilter_hooks.c
> 
> [ ... ]
> 
> > @@ -1178,7 +1178,7 @@ int brnf_sysctl_call_tables(const struct ctl_table *ctl, int write,
> >  	ret = proc_dointvec(ctl, write, buffer, lenp, ppos);
> >  
> >  	if (write && *(int *)(ctl->data))
>                      ^^^^^^^^^^^^^^^^^^^
> > -		*(int *)(ctl->data) = 1;
> > +		WRITE_ONCE(*(int *)(ctl->data), 1);
> 
> [Severity: Low]
> This isn't a bug, but should the load in the condition also be annotated?
> 
> The store is now WRITE_ONCE(), but the check just before it is still a
> plain read of *(int *)(ctl->data).
> 
> proc_sys_call_handler() in fs/proc/proc_sysctl.c calls
> table->proc_handler() without taking any lock. Two concurrent writes to
> the same knob (bridge-nf-call-iptables, for example) can therefore run
> brnf_sysctl_call_tables() in parallel. In that case one handler's plain
> read can race with the other writer's marked store in do_proc_int_conv(),
> or with its WRITE_ONCE(..., 1).
> 
> That is the same plain-read-vs-marked-write pattern as the splat in the
> commit message, so KCSAN would report it as a race between
> brnf_sysctl_call_tables and do_proc_int_conv.
> 
> There is no functional effect here. The final value is always 0 or 1,
> and any non-zero value is treated as true. This only affects how
> complete the annotation is.
> 
> Would it make sense to use READ_ONCE() for the check? Another option is
> to parse into a local value and publish it with a single WRITE_ONCE().

This one looks like it ought to be addressed for completeness.