Re: [PATCH v6 1/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

[sashiko-bot@kernel.org]

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

New issues:
- [High] Non-blocking reads and signal interrupts bypass the fallback to tcp_recvmsg(), breaking read semantics.
- [High] Early return on SOCK_DONE causes data loss and premature EOF.

Pre-existing issues:
- [High] Calling recvmsg() with a length of 0 causes a kernel hang (infinite loop) if data is present.
--

--- Patch [1]: [PATCH v6 1/2] bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup ---
commit 82673cd18298f61fda976da0ad0dad3a80e2c280
Author: Nnamdi Onyeyiri <nnamdio@gmail.com>

bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

recvfrom()/recv() are documented as only returning EAGAIN for blocking
sockets when they have a receive timeout configured. However, adding a
blocking ipv4 tcp socket without a receive timeout to a sockmap will
cause EAGAIN errors sporadically.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 8e905b50deadb..e1c0f8b0930e1 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
[ ... ]
> @@ -262,6 +263,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  		}
>  	}
>  
> +	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> +
>  msg_bytes_ready:
>  	copied = __sk_msg_recvmsg(sk, psock, msg, len, flags, &copied_from_self);
>  	/* The typical case for EFAULT is the socket was gracefully

[Severity: High]
This is a pre-existing issue, but can this loop spin infinitely if a user
calls recv() with a length of 0?

If len is 0, __sk_msg_recvmsg() bypasses its internal loop and returns 0.
The code then enters the if (!copied) branch and calls tcp_msg_wait_data(). 
If there is data in the psock queue, the wait queue condition is met
immediately and it returns 1.

The loop then hits the check:

    if (data && !sk_psock_queue_empty(psock))
        goto msg_bytes_ready;

This unconditionally jumps back to the top of the loop, creating a
CPU-spinning infinite loop that could be triggered by an unprivileged
local user. Does this need an explicit check for len == 0? (This same
pattern exists in tcp_bpf_recvmsg as well).

[ ... ]
> @@ -386,14 +390,59 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  		return tcp_recvmsg(sk, msg, len, flags);
>  	}
>  	lock_sock(sk);
> +
> +	timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> +
>  msg_bytes_ready:
>  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
>  	if (!copied) {
> -		long timeo;
>  		int data;
>  
> -		timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> -		data = tcp_msg_wait_data(sk, psock, timeo);
> +		if (sock_flag(sk, SOCK_DONE)) {
> +			ret = 0;
> +			goto unlock;
> +		}

[Severity: High]
Does this early return cause data loss if unread data remains in the receive
queue after a FIN is received?

When a TCP FIN is received, SOCK_DONE is set on the socket. However, this
does not imply the receive queue is empty; valid data received before the
FIN may still reside in sk->sk_receive_queue.

By returning 0 immediately here, we skip the fallback to tcp_recvmsg() which
would correctly drain the receive queue before returning EOF. Can this strand
and lose unread data?

[ ... ]
> +		if (!timeo) {
> +			ret = -EAGAIN;
> +			goto unlock;
> +		}
> +
> +		if (signal_pending(current)) {
> +			ret = sock_intr_errno(timeo);
> +			goto unlock;
> +		}

[Severity: High]
Do these early returns break non-blocking reads and signal handling for
sockets that have buffered data?

When MSG_DONTWAIT is set, timeo is 0 and this code returns -EAGAIN
immediately. This completely skips the fallback check later in the function:

    if (!skb_queue_empty(&sk->sk_receive_queue)) {
        release_sock(sk);
        ...
        return tcp_recvmsg(sk, ...);
    }

For a socket added to a sockmap without a verdict program, incoming data
buffers in the standard TCP receive queue. Returning -EAGAIN or returning
due to a signal here bypasses reading that buffered data, which seems to
break read semantics for non-blocking sockets.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720171535.67867-1-nnamdio@gmail.com?part=1