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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Calling recv() with a length of 0 on a BPF sockmap TCP socket with pending data and an error state causes an unkillable infinite loop in the kernel.
- [High] tcp_bpf_recvmsg propagates -EFAULT to userspace instead of returning EOF (0) when encountering a 0-length message (such as a redirected FIN).
--

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

bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

This commit fixes spurious wakeups in tcp_msg_wait_data() by looping back
to msg_bytes_ready instead of returning -EAGAIN. It also introduces timeo
pointer passing to avoid excessive waits and ports parser logic over to
tcp_bpf_recvmsg().

> 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

[ ... ]

> @@ -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) {
>  		int data;
>  
> +		if (sock_flag(sk, SOCK_DONE)) {
> +			ret = 0;
> +			goto unlock;
> +		}
> +
> +		if (sk->sk_err) {
> +			if (!sk_psock_queue_empty(psock))
> +				goto msg_bytes_ready;
> +			if (!skb_queue_empty(&sk->sk_receive_queue)) {
> +				release_sock(sk);
> +				sk_psock_put(sk, psock);
> +				return tcp_recvmsg(sk, msg, len, flags);
> +			}
> +			ret = sock_error(sk);
> +			goto unlock;
> +		}
> +
> +		if (sk->sk_shutdown & RCV_SHUTDOWN) {
> +			if (!sk_psock_queue_empty(psock))
> +				goto msg_bytes_ready;

[Severity: High]
This is a pre-existing issue in tcp_bpf_recvmsg_parser(), but this patch
copies the same logic into tcp_bpf_recvmsg(). Does this create an unkillable
infinite loop if the user calls recv() with a length of 0?

If a user waits for data to be queued and ensures an error state is set (for
example, the peer sends a FIN setting RCV_SHUTDOWN), and then calls
recv(fd, buf, 0, 0), sk_msg_recvmsg() immediately returns 0 since len is 0.

The code enters the if (!copied) block, hits the RCV_SHUTDOWN check, and
because !sk_psock_queue_empty(psock) is true, it executes goto
msg_bytes_ready.

Since len is 0, the cycle repeats immediately without releasing the socket
lock or checking for signals, pegging the CPU and potentially causing a
soft lockup. Is there a way to safely break out of this loop when len is 0?

> +			if (!skb_queue_empty(&sk->sk_receive_queue)) {
> +				release_sock(sk);
> +				sk_psock_put(sk, psock);
> +				return tcp_recvmsg(sk, msg, len, flags);
> +			}
> +			ret = 0;
> +			goto unlock;
> +		}

[ ... ]

> @@ -405,6 +454,8 @@ static int tcp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  			sk_psock_put(sk, psock);
>  			return tcp_recvmsg(sk, msg, len, flags);
>  		}
> +		if (!data && timeo > 0)
> +			goto msg_bytes_ready;
>  		copied = -EAGAIN;
>  	}
>  	ret = copied;

[Severity: High]
This is a pre-existing issue, but does tcp_bpf_recvmsg() propagate an
incorrect -EFAULT to userspace when receiving a 0-length message?

If a BPF verdict program redirects a 0-length packet (such as an empty TCP
FIN) to a sockmap socket, it is queued as an sk_msg with sge->length == 0.

When the user calls recv(), __sk_msg_recvmsg() in net/core/skmsg.c
processes the element, sets copy = sge->length (which is 0), and bypasses
copy_page_to_iter(). It then evaluates if (!copy).

Since copy is 0, it triggers copied = copied ? copied : -EFAULT, returning
-EFAULT.

Because -EFAULT is not 0, tcp_bpf_recvmsg() bypasses the if (!copied) error
handling block above and directly assigns ret = copied, returning the
-EFAULT back to the syscall caller. Should this case return EOF (0) instead
of -EFAULT?

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