Re: [PATCH net-next 13/14] netfilter: conntrack: annotate data-races around timeout and tcp sysctls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] netfilter: conntrack: data race in destroy_sibling_or_exp()
  [1]
--

commit 90b17fe93bcd168d06092c32e88d6764be57b60b
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

netfilter: conntrack: annotate data-races around timeout and tcp sysctls

This commit adds READ_ONCE() annotations to packet-path readers of conntrack
timeout and TCP sysctls to avoid data races with concurrent sysctl updates.

[ ... ]

> diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
> index 616ab1e2fc5ed..d8bca1e29797b 100644
> --- a/net/netfilter/nf_conntrack_proto_gre.c
> +++ b/net/netfilter/nf_conntrack_proto_gre.c
> @@ -279,8 +279,8 @@ int nf_conntrack_gre_packet(struct nf_conn *ct,
>  
>  		/* initialize to sane value.  Ideally a conntrack helper
>  		 * (e.g. in case of pptp) is increasing them */
> -		ct->proto.gre.stream_timeout = timeouts[GRE_CT_REPLIED];
> -		ct->proto.gre.timeout = timeouts[GRE_CT_UNREPLIED];
> +		ct->proto.gre.stream_timeout = READ_ONCE(timeouts[GRE_CT_REPLIED]);
> +		ct->proto.gre.timeout = READ_ONCE(timeouts[GRE_CT_UNREPLIED]);
>  	}
>  
>  	status = READ_ONCE(ct->status);

[Severity: Medium]
This problem wasn't introduced by this patch, but can destroy_sibling_or_exp()
cause a data race [1] with nf_conntrack_gre_packet() on
proto.gre.timeout and proto.gre.stream_timeout?

During PPTP teardown, gre_pptp_destroy_siblings() calls
destroy_sibling_or_exp(), which looks up the active GRE conntrack entry with
nf_conntrack_find_get() and zeroes sibling->proto.gre.timeout and
sibling->proto.gre.stream_timeout without synchronization while
nf_conntrack_gre_packet() reads them on another CPU:

CPU0                                    CPU1
gre_pptp_destroy_siblings()             nf_conntrack_gre_packet()
  destroy_sibling_or_exp()
    h = nf_conntrack_find_get(...)
    sibling = nf_ct_tuplehash_to_ctrack(h)
    sibling->proto.gre.timeout = 0;
                                          nf_ct_refresh_acct(...,
                                            ct->proto.gre.timeout);
                                            <-- data race

This unlocked modification can cause inconsistent timeout updates on the
GRE conntrack entry during concurrent traffic.

[1] https://sashiko.dev/#/bug/linux-414ef1d4-4293-47c0-80d0-e03e321aacc5

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109025425.328445-1-jiayuan.chen@linux.dev?part=13