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

[netdev-bot+sashiko@kernel.org]

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().

>  	return ret;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109025425.328445-1-jiayuan.chen%40linux.dev