Re: [RFC PATCH net-next 3/4] tcp: add TCP_TS_ID socket option

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] tcp: lockless data race in do_tcp_getsockopt for TCP_TS_ID
--

commit 7c66b48d6e891deb8097ef54e673d4be713fb65b
Author: Willem Ferguson <wferguson@cloudflare.com>

tcp: add TCP_TS_ID socket option

Add a read-only TCP_TS_ID socket option so applications and tests can see
which TSval identifier was assigned to a connection. The value packs the
identifier width and the identifier itself, chosen by BPF when the SYN
arrives.

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index af994edde7f73..5991b16396b9f 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4711,6 +4711,9 @@ int do_tcp_getsockopt(struct sock *sk, int level,
>  		else
>  			val &= ~1;
>  		break;
> +	case TCP_TS_ID:
> +		val = ((u32)tp->ts_id_bits << TCP_TS_ID_BITS_SHIFT) | tp->ts_id;
> +		break;

[Severity: Low]
Is it possible for tp->ts_id and tp->ts_id_bits to be concurrently modified
here without the socket lock?

Looking at do_tcp_getsockopt(), these fields are read locklessly.
If the socket is concurrently disconnected (for example, by calling
connect() with AF_UNSPEC, which calls tcp_disconnect()), the fields are
zeroed out:

net/ipv4/tcp.c:tcp_disconnect() {
    ...
    tp->ts_id = 0;
    tp->ts_id_bits = 0;
    ...
}

Could this concurrent modification trigger a KCSAN data race warning or
result in returning an inconsistent or torn value to userspace?

Other locklessly accessed fields in this switch statement use READ_ONCE(),
like tp->notsent_lowat right below this. Would it make sense to use
READ_ONCE() here and WRITE_ONCE() in tcp_disconnect() to prevent this?

>  	case TCP_NOTSENT_LOWAT:
>  		val = READ_ONCE(tp->notsent_lowat);
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013103906.2660384-1-wferguson@cloudflare.com?part=3