Re: [PATCH net-next v4 1/2] tcp: annotate lockless access to sk->sk_err

[quanyeyang]

On Friday, September 18th, 2026 at AM 5:27, David Laight <david.laight.linux@xxxxxxxxx> wrote:

> If the race is with locked accesses I think you need both.

Thanks — I think I see the case you mean, and I may have been looking
at a different pairing.

If the stores stay unmarked because they run under a lock, and the
reader is the lockless side, then yes: READ_ONCE() alone is not enough
for KCSAN.  The watchpoint is still the plain locked store, so the
report is marked vs unmarked.  data_race(READ_ONCE()) is the usual
answer there: READ_ONCE() for the compiler, data_race() so KCSAN does
not treat those remaining unmarked locked stores as bugs.

The splat this series is trying to fix looks the other way around,
at least as I read it.  The access KCSAN caught on the write side is
the lockless xchg() in sock_error(), from getsockopt(SO_ERROR) /
recvmmsg.  Those paths do not take lock_sock(), so the peek under
lock_sock() is not actually exclusive against the consumer — the two
sides really do run concurrently.  The peek is a plain load, so that
is the unmarked side of the report.

TCP's own stores of sk_err were already converted to WRITE_ONCE() in
e13ec3da05d1, because tcp_poll() already peeks without the socket
lock.  The consume path is xchg().  Adding READ_ONCE() on the peeks
is meant to mark both sides of that race, same as tcp_poll() already
does.  Wrapping them in data_race() would also silence any leftover
plain stores instead of leaving KCSAN able to point at them.  These
loads also feed send/recv/splice control flow rather than a
diagnostic or heuristic, so READ_ONCE() seemed the closer match.

If you had a specific locked store in mind that we should still treat
as unmarked, or a reason the lock_sock() peek should stay a plain
access, I would like to understand it.

Thanks,
Quanye