Re: [BUG] bpf, sockmap: spurious wakeup by tcp_msg_wait_data() causing unexpected EAGAIN in recvfrom()

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch introduces an unkillable infinite kernel loop (DoS) in `tcp_bpf_recvmsg` when a blocking socket without a receive timeout receives a signal.

Pre-existing issues:
- [High] A race condition during stream parsing causes blocking sockets to prematurely return `-EAGAIN` when data is present.
- [High] The patch fails to fix spurious wakeups for sockets with finite receive timeouts (SO_RCVTIMEO), leaving them susceptible to premature -EAGAIN returns.
--

--- Patch [1]: [BUG] bpf, sockmap: spurious wakeup by tcp_msg_wait_data() causing unexpected EAGAIN in recvfrom() ---
commit 39fdd170f1532151400920cba0bc08398cb63cb8
Author: Nnamdi Onyeyiri <nnamdio@gmail.com>

bpf, sockmap: handle spurious tcp_msg_wait_data() wakeup

This patch attempts to resolve unexpected EAGAIN errors on blocking
ipv4 tcp sockets added to a sockmap by looping on spurious wakeups.
It adds checks in tcp_bpf_recvmsg and tcp_bpf_recvmsg_parser to
jump back to msg_bytes_ready when no data is found and there is no
receive timeout configured.

> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index cc0bd73f36b6d..38fd391ffdb31 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -317,6 +317,8 @@ static int tcp_bpf_recvmsg_parser(struct sock *sk,
>  		}
>  		if (data && !sk_psock_queue_empty(psock))
>  			goto msg_bytes_ready;

[Severity: High]
This is a pre-existing issue, but does this logic incorrectly assume that if
data is present, the parsed data is already in the psock?

If data arrives in sk_receive_queue and triggers a wakeup, but the
thread re-acquires lock_sock before the stream parser can execute and
populate psock->ingress_msg, we bypass the retry and erroneously return
-EAGAIN.

> +		if (!data && timeo == MAX_SCHEDULE_TIMEOUT)
> +			goto msg_bytes_ready;

[Severity: High]
This is a pre-existing issue, but does this skip the fix for sockets
configured with finite timeouts (SO_RCVTIMEO)?

By explicitly checking for MAX_SCHEDULE_TIMEOUT, finite timeouts will
still randomly fail with -EAGAIN on spurious wakeups.

Additionally, because tcp_msg_wait_data() takes timeo by value, the
remaining time calculated by sk_wait_event() is discarded. If this loop
were enabled for finite timeouts, it would reset the timeout on every
iteration.

[ ... ]

> @@ -390,6 +392,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 == MAX_SCHEDULE_TIMEOUT)
> +			goto msg_bytes_ready;

[Severity: Critical]
Could this new unconditional branch cause an infinite kernel loop if the
process receives a signal?

Unlike tcp_bpf_recvmsg_parser(), tcp_bpf_recvmsg() lacks a
signal_pending(current) check inside its loop. If a blocking socket
without a timeout receives a signal, sk_wait_event() returns immediately
without sleeping. 

The new check then jumps back to msg_bytes_ready, causing the kernel 
to spin indefinitely without delivering the signal, resulting in a 
denial of service.

>  		copied = -EAGAIN;
>  	}
>  	ret = copied;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ak_rR-Skd8Mvn4mH@localhost.localdomain?part=1