Re: [PATCH RFC net-next 3/9] tcp: record a start time in the tx path for SK_BPF_CB_TIMESTAMPING_V2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] tcp: concurrent sendmsg corrupts per-syscall sk_start_time
--

commit c712b000347bc5edb0a47ae0ad843a63f0d66b24
Author: Jason Xing <kerneljasonxing@gmail.com>

tcp: record a start time in the tx path for SK_BPF_CB_TIMESTAMPING_V2

This commit adds a new sk_start_time field to struct sock to record the
syscall start time for SK_BPF_CB_TIMESTAMPING_V2. The time is recorded on
entry to tcp_sendmsg_locked() for later propagation into skbs.

> diff --git a/include/net/sock.h b/include/net/sock.h
> index 60ea55dc18854..30e7894c15b43 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
[ ... ]
> @@ -552,6 +553,7 @@ struct sock {
>  #if BITS_PER_LONG==32
>  	seqlock_t		sk_stamp_seq;
>  #endif
> +	ktime_t			sk_start_time;

[Severity: Medium]
Is struct sock the appropriate place to store a per-syscall state?

Since this field is in a shared structure, it could be vulnerable to being
overwritten by concurrent calls. Could this time be passed down via a local
execution context like struct sockcm_cookie instead?

>  	int			sk_disconnects;
>  
>  	union {
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 3ac4856852794..f07abd53ed27f 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -1132,6 +1132,10 @@ int tcp_sendmsg_locked(struct sock *sk, struct msghdr *msg, size_t size)
>  
>  	flags = msg->msg_flags;
>  
> +	if (static_branch_unlikely(&bpfts_v2_needed_key) &&
> +	    SK_BPF_CB_FLAG_TEST(sk, SK_BPF_CB_TIMESTAMPING_V2))
> +		sk->sk_start_time = ktime_get_real();
> +

[Severity: Medium]
Can concurrent sendmsg() calls corrupt this per-syscall start time?

If the current sendmsg() blocks later in tcp_sendmsg_locked() (for example,
while waiting for memory in sk_stream_wait_memory() or for a connection in
sk_stream_wait_connect()), the socket lock is temporarily released.

If another thread executes a concurrent sendmsg() on the same socket, it
will acquire the lock and overwrite sk->sk_start_time here. When the first
thread resumes execution, won't any new SKBs it allocates incorrectly
inherit the second thread's start time?

>  	sockc = (struct sockcm_cookie){ .tsflags = READ_ONCE(sk->sk_tsflags) };
>  	if (msg->msg_controllen) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919143732.11772-1-kerneljasonxing@gmail.com?part=3