Re: [PATCH bpf] bpf, sockmap: take the socket lock in udp_bpf_recvmsg()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `udp_bpf_recvmsg` returns `-EAGAIN` instead of `-EINTR` or `-ERESTARTSYS` on signal interruption for blocking sockets.
- [High] `udp_bpf_recvmsg` enters an infinite busy-wait loop if data is queued in `psock->ingress_skb` but not yet processed into `psock->ingress_msg`.
- [High] Lost wakeups and deadlocks for BPF-redirected packets when the UDP socket is initially empty.
- [High] `vsock_bpf_recvmsg` sleeps while holding the socket lock, causing a deadlock with incoming packet delivery.
--

commit 093cedb7b7f9c8e7af25b932fdeb58d606fcc2ea
Author: Sechang Lim <rhkrqnwk98@gmail.com>

bpf, sockmap: take the socket lock in udp_bpf_recvmsg()

udp_bpf_recvmsg() drains the psock ingress_msg queue through
sk_msg_recvmsg() without holding the socket lock. This commit
adds lock_sock() and release_sock() around the consume path
so a given ingress sk_msg is drained by one reader at a time.

> diff --git a/net/ipv4/udp_bpf.c b/net/ipv4/udp_bpf.c
> index 9f33b07b14813..8dd73e9ee1ea6 100644
> --- a/net/ipv4/udp_bpf.c
> +++ b/net/ipv4/udp_bpf.c

[Severity: High]
This isn't a bug introduced by this patch, but can the unconditional
delegation to sk_udp_recvmsg() in udp_bpf_recvmsg() cause lost wakeups
and deadlocks for BPF-redirected packets when the UDP socket is
initially empty?

If !psock_has_data(psock) is initially true, udp_bpf_recvmsg() does:

    if (!psock_has_data(psock)) {
        ret = sk_udp_recvmsg(sk, msg, len, flags);
        goto out;
    }

sk_udp_recvmsg() will sleep on the native socket wait queue checking
only sk->sk_receive_queue.

If a packet arrives and is redirected by a BPF program to
psock->ingress_msg, the wait queue is woken up, but because
sk_receive_queue is still empty, the native UDP wait loop
(__skb_wait_for_more_packets) will immediately go back to sleep.

Does this leave the packet stuck in psock and the user thread hanging
permanently?

> @@ -80,7 +80,9 @@ static int udp_bpf_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  	}
>  
>  msg_bytes_ready:
> +	lock_sock(sk);
>  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
> +	release_sock(sk);

[Severity: High]
This is a pre-existing issue, but can this code enter an infinite
busy-wait loop if data is queued in psock->ingress_skb but not yet
processed into psock->ingress_msg?

If psock->ingress_skb is not empty, psock_has_data() evaluates to true.

Then udp_bpf_recvmsg() calls sk_msg_recvmsg(), which only reads from
ingress_msg, returning 0.

Because psock_has_data() is true, udp_msg_wait_data() returns 1
without sleeping. The loop sees data == 1 and psock_has_data() == true
and jumps back to msg_bytes_ready.

Does this create a tight spin loop that rapidly acquires and releases
lock_sock, causing CPU starvation until the backlog workqueue thread
finishes moving the skb?

>  	if (!copied) {
>  		long timeo;
>  		int data;

[Severity: High]
This is a pre-existing issue, but does udp_bpf_recvmsg() violate POSIX
semantics by returning -EAGAIN instead of -EINTR or -ERESTARTSYS on
signal interruption for blocking sockets?

If a blocking recvmsg is interrupted by a signal, wait_woken() inside
udp_msg_wait_data() returns prematurely, causing udp_msg_wait_data()
to return 0.

The code then unconditionally sets copied = -EAGAIN when data is 0:

    copied = -EAGAIN;

Does this completely ignore signal_pending(current)?

Additionally, there is a pre-existing issue in net/vmw_vsock/vsock_bpf.c:

[Severity: High]
This isn't a bug introduced by this patch, but does vsock_bpf_recvmsg()
sleep while holding the socket lock, causing a deadlock with incoming
packet delivery?

In vsock_bpf_recvmsg(), the code acquires lock_sock(sk) and calls
vsock_msg_wait_data() when no data is available:

vsock_bpf_recvmsg()
    lock_sock(sk);
    ...
    while (copied == 0) {
        long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
        if (!vsock_msg_wait_data(sk, psock, timeo)) {

This function calls wait_woken() without releasing the socket lock:

vsock_msg_wait_data()
    wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);

Meanwhile, the Rx thread (virtio_transport_recv_pkt()) needs to acquire
lock_sock(sk) to deliver incoming packets and trigger sk_data_ready().

Because the sleeping user task holds the lock, does the Rx thread block
indefinitely, resulting in a deadlock where the user task is never
woken up?

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