Re: [PATCH v3 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] Socket state checks in `tcp_bpf_recvmsg` bypass unread data in `sk_receive_queue`, causing data loss on concurrent socket teardown or errors.

Pre-existing issues:
- [High] Sporadic EAGAIN can still occur on blocking sockets with a verdict parser due to a race with the BPF parser workqueue.
- [High] `tcp_bpf_recvmsg()` incorrectly returns -EAGAIN instead of reading redirected data after waking up.
--

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

bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

This commit modifies tcp_bpf_recvmsg() and tcp_bpf_recvmsg_parser() to handle
spurious wakeups from tcp_msg_wait_data(). It updates sk_wait_event() to
maintain the timeout by passing it as a pointer. The commit also introduces
socket state checks into tcp_bpf_recvmsg() to match tcp_bpf_recvmsg_parser().

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index cc0bd73f36b6d..e58ab2d1ee943 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c

[ ... ]

> @@ -310,13 +311,15 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  			goto out;
>  		}
>  
> -		data = tcp_msg_wait_data(sk, psock, timeo);
> +		data = tcp_msg_wait_data(sk, psock, &timeo);
>  		if (data < 0) {
>  			copied = data;
>  			goto unlock;
>  		}
>  		if (data && !sk_psock_queue_empty(psock))
>  			goto msg_bytes_ready;
> +		if (!data && timeo > 0)
> +			goto msg_bytes_ready;
>  		copied = -EAGAIN;
>  	}

[Severity: High]
This is a pre-existing issue, but does this logic still sporadically return
-EAGAIN on blocking sockets when data == 1?

If a blocking recv() is called on a sockmap socket with a verdict parser, and a
packet arrives in sk_receive_queue, tcp_msg_wait_data() will return 1.

However, if the BPF workqueue hasn't yet processed the data into
psock->ingress_msg, sk_psock_queue_empty(psock) could still be true.

It looks like the loop fails to handle data == 1 with an empty psock, bypassing
the new !data check and falling through to return -EAGAIN.

[ ... ]

> @@ -371,14 +375,45 @@ 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;
>  
> -		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;
> +		}
> +
> +		if (sk->sk_err) {
> +			ret = sock_error(sk);
> +			goto unlock;
> +		}
> +
> +		if (sk->sk_shutdown & RCV_SHUTDOWN) {
> +			ret = 0;
> +			goto unlock;
> +		}

[Severity: High]
Does evaluating these state flags before falling back to tcp_recvmsg() cause
data loss?

If normal data arrives and is queued in sk_receive_queue, and a FIN
(RCV_SHUTDOWN) or RST (sk_err) arrives immediately after, recv() would see
psock->ingress_msg as empty.

It seems like this newly added code evaluates the state flags and immediately
returns 0 or an error, bypassing the fallback to tcp_recvmsg() which would have
correctly drained the unread data.

[ ... ]

>  		data = tcp_msg_wait_data(sk, psock, &timeo);
>  		if (data < 0) {
>  			ret = data;
>  			goto unlock;
>  		}
>  		if (data && !skb_queue_empty_lockless(&sk->sk_receive_queue)) {
>  			sk_psock_put(sk, psock);
>  			return tcp_recvmsg(sk, msg, len, flags);
>  		}
> +		if (!data && timeo > 0)
> +			goto msg_bytes_ready;
>  		copied = -EAGAIN;
>  	}

[Severity: High]
This is a pre-existing issue, but could this return -EAGAIN instead of reading
redirected data?

If data == 1 because of redirected data in psock->ingress_msg, but
sk_receive_queue is empty, the condition:

    if (data && !skb_queue_empty_lockless(&sk->sk_receive_queue)) {

will be false.

The next check:

    if (!data && timeo > 0)

will also be false.

It seems the code then falls through to return -EAGAIN instead of looping back
to read the redirected data from the psock.

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