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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (2) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Commit message does not describe the tcp_bpf_sendmsg() hunk.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the
  annotated peeks are still followed by a second, independent read of…
- [Medium] This problem wasn't introduced by this patch, but
  tcp_bpf_sendmsg() (net/ipv4/tcp_bpf.c) discards a failed iterator-copy…

--

tcp: annotate lockless access to sk->sk_err

This annotates the plain sk->sk_err loads on the TCP send, receive and
splice paths with READ_ONCE(), since sock_error() clears sk_err with
xchg() from do_recvmmsg() and getsockopt(SO_ERROR) without holding the
socket lock. The tcp_bpf_sendmsg() site is also restructured while the
other sites are annotated in place.

Link: https://lore.kernel.org/netdev/8bbee583-6f21-4817-bfeb-2d60057380a3@xxxxxxxxx/
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/632

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 1c867a3024446..9e7fa7e397265 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -843,7 +843,7 @@ ssize_t tcp_splice_read(struct socket *sock, loff_t *ppos,
>  				break;
>  			if (sock_flag(sk, SOCK_DONE))
>  				break;
> -			if (sk->sk_err) {
> +			if (READ_ONCE(sk->sk_err)) {
>  				ret = sock_error(sk);
>  				break;
>  			}

[ ... ]

> @@ -2760,7 +2760,7 @@ static int tcp_recvmsg_locked(struct sock *sk, struct msghdr *msg, size_t len,
>  			if (sock_flag(sk, SOCK_DONE))
>  				break;
>  
> -			if (sk->sk_err) {
> +			if (READ_ONCE(sk->sk_err)) {
>  				copied = sock_error(sk);
>  				break;
>  			}

[Severity: Medium]
This isn't a bug introduced by this patch, but the annotated peek is
still followed by a second, independent read of sk_err inside
sock_error(), so the value can be gone by the time it is consumed.

include/net/sock.h:sock_error() {
	if (likely(data_race(!sk->sk_err)))
		return 0;

	err = xchg(&sk->sk_err, 0);
	return -err;
}

sock_error() is reachable without the socket lock from do_recvmmsg() and
from getsockopt(SO_ERROR), which is the same writer named in the splat:

CPU0 (socket lock held, tcp_recvmsg_locked())
	if (READ_ONCE(sk->sk_err))	/* sees 0x6b, enters branch */

CPU1 (no socket lock, do_recvmmsg() -> sock_error())
	err = xchg(&sk->sk_err, 0);	/* consumes the error */

CPU0 continues:
		copied = sock_error(sk);	/* now returns 0 */
		break;

Since the break is unconditional, does recvmsg() then return 0 rather
than falling through to the RCV_SHUTDOWN / TCP_CLOSE / !timeo / signal
checks?  For a nonblocking receive on an open, empty socket the expected
result would be -EAGAIN, and userspace reads 0 as an orderly shutdown.

The same shape exists in tcp_splice_read(), where ret = sock_error(sk)
followed by "if (spliced) return spliced; return ret;" returns 0, and in
tcp_bpf_recvmsg_parser(), where copied = sock_error(sk) is followed by
goto out.

The changelog does say "the check-then-sock_error() TOCTOU on the
no-data paths is a separate issue".  Would consuming the value once
(copied = sock_error(sk); if (copied) break;) close both the data race
and this window in one step?

One related note: once these loads are marked, KCSAN will no longer
report these sites, so the signal that produced this report goes away
while the check-then-consume window stays.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 2e234d155b5e6..a96323e65758e 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -286,7 +286,7 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  		if (sock_flag(sk, SOCK_DONE))
>  			goto out;
>  
> -		if (sk->sk_err) {
> +		if (READ_ONCE(sk->sk_err)) {
>  			copied = sock_error(sk);
>  			goto out;
>  		}
> @@ -553,10 +553,9 @@ static int tcp_bpf_sendmsg(struct sock *sk, struct msghdr *msg, size_t size)
>  		bool enospc = false;
>  		u32 copy, osize;
>  
> -		if (sk->sk_err) {
> -			err = -sk->sk_err;
> +		err = -READ_ONCE(sk->sk_err);
> +		if (err)
>  			goto out_err;
> -		}

[Severity: Medium]
Could the changelog cover this hunk?  It describes the patch as a pure
annotation:

  "Annotate those peeks with READ_ONCE(). No extra ordering is needed:
   the value is only used to decide whether I/O should stop. This does
   not consume sk_err"

In tcp_bpf_sendmsg() the value is not only used to decide whether I/O
stops, it becomes the errno handed back to userspace:

net/ipv4/tcp_bpf.c:tcp_bpf_sendmsg() {
	...
out_err:
	if (err < 0)
		err = sk_stream_error(sk, msg->msg_flags, err);
	release_sock(sk);
	sk_psock_put(sk, psock);
	return copied > 0 ? copied : err;
}

The rewrite also looks like it changes behaviour beyond annotation.  The
old form read sk->sk_err twice:

		if (sk->sk_err) {
			err = -sk->sk_err;

If the lockless sock_error() consumer cleared sk_err between those two
reads, err became 0 while control still jumped to out_err, where
"if (err < 0)" is false and the function returned
"copied > 0 ? copied : 0", i.e. a zero-byte success instead of the
pending error.  The new single READ_ONCE() closes that.  Should this be
called out in the changelog, with a Fixes: tag, or split into its own
patch so the remaining five hunks stay mechanical?

Also, err is now assigned on every loop iteration rather than only when
sk_err is set.  Every later use of err in the loop is preceded by its
own assignment from sk_msg_alloc(), tcp_bpf_send_verdict() or
sk_stream_wait_memory(), so this looks behaviour preserving, but it is
unexplained restructuring in an annotation-only patch.

[Severity: Medium]
This is a pre-existing issue, but while looking at the same loop in
tcp_bpf_sendmsg(): can a failed copy from the user buffer be reported as
a zero-byte send?

The copy result goes into ret, not err, and out_err returns err:

net/ipv4/tcp_bpf.c:tcp_bpf_sendmsg() {
	...
		err = sk_msg_alloc(sk, msg_tx, msg_tx->sg.size + copy, msg_tx->sg.end - 1);
		if (err) {
			if (err != -ENOSPC)
				goto wait_for_memory;
			enospc = true;
			copy = msg_tx->sg.size - osize;
		}

		ret = sk_msg_memcopy_from_iter(sk, &msg->msg_iter, msg_tx,
					       copy);
		if (ret < 0) {
			sk_msg_trim(sk, msg_tx, osize);
			goto out_err;
		}
	...
}

sk_msg_memcopy_from_iter() returns -EFAULT on a short copy:

net/core/skmsg.c:sk_msg_memcopy_from_iter() {
	...
		if (ret != copy) {
			ret = -EFAULT;
			goto out;
		}
	...
}

On the first iteration a successful sk_msg_alloc() leaves err == 0, so
out_err skips sk_stream_error() and, with copied == 0, sendmsg() returns
0 rather than -EFAULT.  If sk_msg_alloc() had returned the tolerated
-ENOSPC, the stale -ENOSPC is reported instead of the copy failure.
Would "err = ret;" before the goto preserve the real failure?

Reproducing this needs a sockmap/psock attached to the socket, then a
sendmsg() with a partially unmapped user buffer.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917-mptcp-sk-err-net-v4-0-1f04f52f2561%40proton.me