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

[netdev-bot+sashiko@kernel.org]

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?

>  
>  	if (tb[CTA_TIMEOUT_UDP_UNREPLIED]) {
> -		timeouts[UDP_CT_UNREPLIED] =
> -			ntohl(nla_get_be32(tb[CTA_TIMEOUT_UDP_UNREPLIED])) * HZ;
> +		WRITE_ONCE(timeouts[UDP_CT_UNREPLIED],
> +			   ntohl(nla_get_be32(tb[CTA_TIMEOUT_UDP_UNREPLIED])) * HZ);
>  	}

[ ... ]

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