Re: [PATCH net v2] 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: the patch converts only the sk_err
  load inside tcp_recv_should_stop() (include/net/tcp.h:3086) to…

--

tcp: fix data-race in tcp_recv_should_stop

This converts the sk_err load inside tcp_recv_should_stop() to READ_ONCE()
because do_recvmmsg() calls sock_error() without the socket lock and
sock_error() clears sk_err with xchg(). It carries a Fixes: tag for
eb477fdd6803 and a Closes: tag for the MPTCP syzkaller report.

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 436495ff2271..c61d8678eafd 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]
Is annotating only this read enough to close the reported race?

Every caller of tcp_recv_should_stop() keeps a plain read of the same
field in the other arm of the very same loop.  In mptcp_recvmsg(), the
function named in the KCSAN splat:

net/mptcp/protocol.c:mptcp_recvmsg() {
	...
		if (copied) {
			if (tcp_recv_should_stop(sk) ||
			    !timeo)
				break;
		} else {
			if (sk->sk_err) {
				copied = sock_error(sk);
				break;
			}
	...
}

The same shape exists in tcp_recvmsg_locked():

net/ipv4/tcp.c:tcp_recvmsg_locked() {
	...
		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;
			}
	...
}

and in tcp_splice_read() and mptcp_splice_read(), whose loops both end
with the now-annotated helper:

net/ipv4/tcp.c:tcp_splice_read() {
	...
			if (sk->sk_err) {
				ret = sock_error(sk);
				break;
			}
	...
		if (tcp_recv_should_stop(sk))
			break;
	...
}

The writer described in the changelog, do_recvmmsg() calling
sock_error(sock->sk) with no lock_sock() and clearing the field with
xchg(&sk->sk_err, 0), races with those four plain reads exactly as it
raced with the one that was annotated, since lock_sock() in the readers
gives no exclusion against it.

Would a reproducer that lands in the blocking, copied == 0 arm still
produce the identical "data-race in do_recvmmsg / mptcp_recvmsg" report,
just attributed to mptcp_recvmsg() rather than to
tcp_recv_should_stop()?  If so, should the Closes: tag be dropped, or
should the four remaining reads be converted in the same patch?

The Fixes: tag points at eb477fdd6803, but that commit only moved these
pre-existing plain reads into the new helper; the sibling reads left
behind are part of the same defect.  Does the Fixes: attribution still
hold with only the helper annotated?

The added comment states "sk_err can be cleared locklessly by
sock_error()", which reads as a property of the field rather than of this
one call site.  Should the write side conventions be noted too, since
the writers in these files already use WRITE_ONCE() and sock_error()
itself uses data_race() for its own read?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909-mptcp-sk-err-net-v2-1-5044abecac90%40proton.me