Re: [PATCH net-next 14/14] netfilter: cttimeout: annotate data-races around the default timeouts

[Simon Horman <horms@kernel.org>]

On Tue, Nov 10, 2026 at 05:53:27AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] A concurrent sysctl write to a default conntrack timeout can
>   still be silently undone.
> 
> --
> 
> netfilter: cttimeout: annotate data-races around the default timeouts
> 
> This commit changes the l4proto nlattr_to_obj() and obj_to_nlattr()
> timeout callbacks to use WRITE_ONCE() and READ_ONCE() instead of plain
> accesses. It targets races between cttimeout_default_set() and
> cttimeout_default_get(), the conntrack packet path, and the
> nf_conntrack_*_timeout* sysctl handlers, which all touch the same
> per-netns default timeout arrays.
> 
> > diff --git a/net/netfilter/nf_conntrack_proto_udp.c b/net/netfilter/nf_conntrack_proto_udp.c
> > index 99eedaa2b1183..777b21da3bcff 100644
> > --- a/net/netfilter/nf_conntrack_proto_udp.c
> > +++ b/net/netfilter/nf_conntrack_proto_udp.c
> > @@ -155,16 +155,16 @@ static int udp_timeout_nlattr_to_obj(struct nlattr *tb[],
> >  		timeouts = un->timeouts;
> >  
> >  	/* set default timeouts for UDP. */
> > -	timeouts[UDP_CT_UNREPLIED] = un->timeouts[UDP_CT_UNREPLIED];
> > -	timeouts[UDP_CT_REPLIED] = un->timeouts[UDP_CT_REPLIED];
> > +	WRITE_ONCE(timeouts[UDP_CT_UNREPLIED], READ_ONCE(un->timeouts[UDP_CT_UNREPLIED]));
> > +	WRITE_ONCE(timeouts[UDP_CT_REPLIED], READ_ONCE(un->timeouts[UDP_CT_REPLIED]));
> 
> [Severity: Medium]
> 
> Can a concurrent sysctl write still be lost here?
> 
> cttimeout_default_set() passes a NULL destination:
> 
> net/netfilter/nfnetlink_cttimeout.c:cttimeout_default_set() {
>     ...
> 	ret = ctnl_timeout_parse_policy(NULL, l4proto, info->net,
> 					cda[CTA_TIMEOUT_DATA]);
>     ...
> }
> 
> So udp_timeout_nlattr_to_obj() sets timeouts to un->timeouts, and the
> default copy turns into a self-copy of the live per-netns array:
> 
> 	WRITE_ONCE(un->timeouts[UDP_CT_REPLIED],
> 		   READ_ONCE(un->timeouts[UDP_CT_REPLIED]));
> 
> That is still a separate load and store. The sysctl handler writes the
> same field through proc_dointvec_jiffies() and does not take the nfnl
> mutex. Take a request that carries only CTA_TIMEOUT_UDP_UNREPLIED, sent
> while nf_conntrack_udp_timeout_stream is being written:
> 
> CPU0 (IPCTNL_MSG_TIMEOUT_DEFAULT_SET)
>   old = READ_ONCE(un->timeouts[UDP_CT_REPLIED])
> 
> CPU1 (sysctl write)
>   WRITE_ONCE(un->timeouts[UDP_CT_REPLIED], new)
> 
> CPU0
>   WRITE_ONCE(un->timeouts[UDP_CT_REPLIED], old)
> 
> The sysctl value gets reverted, even though the netlink request never
> named that attribute.
> 
> The second KCSAN splat in the commit message (proc_int_u2k_conv_uop vs
> udp_timeout_nlattr_to_obj) comes from this access pair. Once both sides
> are marked, KCSAN stops reporting it, but the update can still be lost.
> 
> The same self-copy happens in these places:
> 
>   tcp_timeout_nlattr_to_obj(), where timeouts = tn->timeouts and the
>   loop covers all TCP_CONNTRACK_TIMEOUT_MAX entries
>   sctp_timeout_nlattr_to_obj(), where timeouts = sn->timeouts
>   gre_timeout_nlattr_to_obj(), where timeouts = gre_get_timeouts(net)
>   the else branch of generic_timeout_nlattr_to_obj(), when timeout is
>   &gn->timeout
>   the else branch of icmpv6_timeout_nlattr_to_obj(), when timeout
>   falls back to icmpv6_get_timeouts(net)
> 
> When the destination already points at the per-netns array, could the
> default copy be skipped, so that fields missing from the request are
> never stored?

This also looks like it should be addressed.

-- 
pw-bot: changes-requested