Re: [PATCH mptcp-net v3 3/3] tcp: fix data-race in do_recvmmsg / mptcp_recvmsg

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incomplete fix for `sk->sk_err` data race leading to false EOFs. The patch fixes a lockless data race in `tcp_recv_should_stop()` by adding `READ_ONCE()`, but overlooks identical plain reads followed by `sock_error()` in `tcp_recvmsg_locked()`, `tcp_splice_read()`, `mptcp_recvmsg()`, and `tcp_bpf_sendmsg()`. If `sk->sk_err` is checked and evaluates to true, but is concurrently cleared to 0 by another thread calling `sock_error()` (e.g. via `do_recvmmsg` or `getsockopt(SO_ERROR)`), the subsequent assignment `copied = sock_error(sk)` will evaluate to 0. For receive and splice functions, this results in returning 0, which falsely signals a graceful connection closure (EOF) to the application rather than an error.
--

commit 0f561938f67b19245ef8889cb9ecc077fb82d7d0
Author: Quanye Yang <quanyeyang@proton.me>

tcp: fix data-race in do_recvmmsg / mptcp_recvmsg

This commit fixes a lockless data race in tcp_recv_should_stop() by using
READ_ONCE() for reading sk->sk_err. This prevents issues when sock_error()
atomically clears the error using xchg() without holding the socket lock.

> 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) ||

[Severity: High]
This is a pre-existing issue, but does this fix also need to be applied to
other locations that read sk->sk_err locklessly?

If sk->sk_err is evaluated as non-zero, but the error is concurrently cleared
by another thread calling sock_error() (for example, via concurrent recvmmsg
or getsockopt calls), a subsequent call to sock_error(sk) will return 0. This
masks the socket error and falsely signals a graceful connection closure (EOF)
or 0 bytes sent.

This pattern appears to be present in several other functions:

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

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

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

net/ipv4/tcp_bpf.c:tcp_bpf_sendmsg() {
...
    if (sk->sk_err) {
        err = -sk->sk_err;
        goto out_err;
    }
...
}

Would it be appropriate to update these instances as well to prevent
Time-Of-Check to Time-Of-Use races?

>  	       sk->sk_state == TCP_CLOSE ||
>  	       (sk->sk_shutdown & RCV_SHUTDOWN) ||
>  	       signal_pending(current);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-mptcp-snd-una-race-v3-0-65ba0d5f0221@proton.me?part=3