Re: [PATCH net v2] tcp: fix data-race in tcp_recv_should_stop
flat view
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-10 02:56:23
Also in:
lkml
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/ (local)
quoted hunk ↗ jump to hunk
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