Re: [PATCH net] tcp: fix data-race in tcp_recv_should_stop

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Incomplete data-race fix: only the sk_err load inside
  tcp_recv_should_stop() is annotated, while every caller of the helper…

--

tcp: fix data-race in tcp_recv_should_stop

This converts the sk_err load in the tcp_recv_should_stop() helper to
READ_ONCE(), since do_recvmmsg() calls sock_error() without the socket
lock and clears sk_err with xchg(). A KCSAN report on an MPTCP socket
is quoted in the changelog.

Link: https://lore.kernel.org/all/14749060-d011-41e7-9a4b-754eb5bd9d5b@redhat.com/

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 436495ff2271d..c61d8678eafd3 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -3082,7 +3082,8 @@ enum skb_drop_reason tcp_inbound_hash(struct sock *sk,
>  
>  static inline int tcp_recv_should_stop(struct sock *sk)
>  {
> -	return sk->sk_err ||
> +	/* sk_err can be cleared locklessly by sock_error(). */
> +	return READ_ONCE(sk->sk_err) ||
>  	       sk->sk_state == TCP_CLOSE ||
>  	       (sk->sk_shutdown & RCV_SHUTDOWN) ||
>  	       signal_pending(current);

[Severity: High]
Does annotating only the helper close the race the changelog describes?
Every caller of tcp_recv_should_stop() still has a plain load of the same
field in the same wait loop, a few lines below the helper call.

net/mptcp/protocol.c:mptcp_recvmsg() - the function named in the quoted
report:

	if (copied) {
		if (tcp_recv_should_stop(sk) ||
		    !timeo)
			break;
	} else {
		if (sk->sk_err) {
			copied = sock_error(sk);
			break;
		}

net/ipv4/tcp.c:tcp_recvmsg_locked() has the identical pair:

		if (copied) {
			if (!timeo ||
			    tcp_recv_should_stop(sk))
				break;
		} else {
			if (sock_flag(sk, SOCK_DONE))
				break;

			if (sk->sk_err) {
				copied = sock_error(sk);
				break;
			}

The same unmarked load also remains in tcp_splice_read() and in
mptcp_splice_read(), which are the other two callers of the helper.

The racing writer is unchanged in all four cases:

net/socket.c:do_recvmmsg() {
	...
	if (likely(!(flags & MSG_ERRQUEUE))) {
		err = sock_error(sock->sk);
	...
}

which runs with no socket lock and does xchg(&sk->sk_err, 0) in
sock_error(). So a thread in recvmsg() on the copied == 0 path and a
second thread in recvmmsg() on the same socket still produce the marked
read-write versus unmarked read pair, and the splat looks like it would
just be re-reported from mptcp_recvmsg() and tcp_recvmsg_locked()
directly instead of from the inlined helper.

Would it make sense to convert those four sibling loads to READ_ONCE()
in the same patch?

The changelog only mentions the leftover problem as:

    This does not consume sk_err; the check-then-sock_error() TOCTOU
    on the no-data paths is a separate issue.

That describes a semantic check-then-act concern, which is distinct from
the plain concurrent load. Could the changelog say explicitly which
sk_err loads remain unmarked after this change?

Converting the sibling reads would also make the change applicable to
trees that predate the Fixes: eb477fdd6803 helper, where only those reads
exist.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908-mptcp-sk-err-net-v1-1-da71aaec9afd%40proton.me