* Re: [PATCH net] vsock/bpf: release sock lock while waiting for data in recvmsg
2026-09-29 16:33 [PATCH net] vsock/bpf: release sock lock while waiting for data in recvmsg Jerome Mohm via B4 Relay
@ 2026-10-01 9:01 ` Stefano Garzarella
2026-10-01 16:35 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: Stefano Garzarella @ 2026-10-01 9:01 UTC (permalink / raw)
To: jrmmhm.kernel, Michal Luczaj, Bobby Eshleman
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Simon Horman, Bobby Eshleman, Michael S. Tsirkin, virtualization,
netdev, linux-kernel, bpf, stable
+cc Bobby and Michal who touched this code recently
On Tue, Sep 29, 2026 at 06:33:04PM +0200, Jerome Mohm via B4 Relay wrote:
>From: Jerome Mohm <jrmmhm.kernel@eldare.de>
>
>vsock_bpf_recvmsg() takes lock_sock(sk) and holds it across the whole
>receive loop, including vsock_msg_wait_data(), which sleeps in
>wait_woken() without dropping the lock. When data arrives, the
>transport's delivery context (the vsock-loopback worker, or the
>virtio/vhost rx path) calls virtio_transport_recv_pkt() -> lock_sock()
>on the same socket and blocks. That context is the only one that would
>queue the skb and call sk_data_ready() to wake the reader, so neither
>side makes progress. A blocking recv() hangs until a signal arrives (a
>finite SO_RCVTIMEO also breaks it); while it lasts the shared delivery
>worker is stalled, so all vsock rx on that transport stops, not only
>the affected socket. The hung-task watchdog reports the worker blocked
>in D state:
>
> INFO: task kworker/1:3:107 blocked for more than 5 seconds.
> task:kworker/1:3 state:D Workqueue: vsock-loopback vsock_loopback_work
> Call Trace:
> __schedule
> schedule
> __lock_sock
> lock_sock_nested
> virtio_transport_recv_pkt
> vsock_loopback_work
>
>The reader holds the same sk_lock-AF_VSOCK it is waiting on, taken in
>vsock_bpf_recvmsg().
>
>This code is based on net/unix/unix_bpf.c, whose unix_msg_wait_data()
>drops u->iolock around wait_woken() and re-takes it afterwards. The
>vsock port substituted lock_sock() for that serialisation lock but
>omitted the unlock and relock. tcp_bpf and the native
>vsock_connectible_wait_data() both drop the lock across the wait;
>vsock_bpf is the only one that does not.
>
>Release the socket lock around the wait and re-acquire it before
>re-checking for data, so the caller's locking is unchanged.
>
>Fixes: 634f1a7110b4 ("vsock: support sockmap")
>Cc: stable@vger.kernel.org
>Assisted-by: LLM
>Signed-off-by: Jerome Mohm <jrmmhm.kernel@eldare.de>
>---
> net/vmw_vsock/vsock_bpf.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
>index 9049d2648646..bb7d81a95baa 100644
>--- a/net/vmw_vsock/vsock_bpf.c
>+++ b/net/vmw_vsock/vsock_bpf.c
>@@ -50,7 +50,9 @@ static bool vsock_msg_wait_data(struct sock *sk, struct sk_psock *psock, long ti
> sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> ret = vsock_has_data(sk, psock);
> if (!ret) {
>+ release_sock(sk);
> wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
>+ lock_sock(sk);
> ret = vsock_has_data(sk, psock);
> }
> sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>
>---
>base-commit: 72d3fcf802c45d00b300f25b848a93c3a2bd7c7e
>change-id: 20260929-kbh3-1-022-fix-340aa3f7f071
>
>Best regards,
>--
>Jerome Mohm <jrmmhm.kernel@eldare.de>
>
LGTM, but I'd like also Bobby and Michal opinion:
Acked-by: Stefano Garzarella <sgarzare@redhat.com>
Thanks,
Stefano
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] vsock/bpf: release sock lock while waiting for data in recvmsg
2026-09-29 16:33 [PATCH net] vsock/bpf: release sock lock while waiting for data in recvmsg Jerome Mohm via B4 Relay
2026-10-01 9:01 ` Stefano Garzarella
@ 2026-10-01 16:35 ` netdev-bot+sashiko
1 sibling, 0 replies; 3+ messages in thread
From: netdev-bot+sashiko @ 2026-10-01 16:35 UTC (permalink / raw)
To: jrmmhm.kernel
Cc: sgarzare, davem, edumazet, kuba, pabeni, horms, bobby.eshleman,
mst, virtualization, netdev, linux-kernel, bpf, stable
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 (2) · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] Because the patch releases the socket lock around wait_woken()
in vsock_msg_wait_data() (net/vmw_vsock/vsock_bpf.c:52-57), the virtio…
Pre-existing issues:
- [High] In net/vmw_vsock/vsock_bpf.c, vsock_msg_wait_data() stops
waiting only when the local socket has RCV_SHUTDOWN set (line 43: `if…
- [High] vsock_bpf_recvmsg() in net/vmw_vsock/vsock_bpf.c has no zero-
length early return.
--
vsock/bpf: release sock lock while waiting for data in recvmsg
This commit drops the socket lock around wait_woken() in
vsock_msg_wait_data(). The transport rx path, virtio_transport_recv_pkt(),
can then take lock_sock() and deliver data while a sockmap reader sleeps.
Before this change, a blocking recv() and the shared vsock delivery worker
deadlocked on the same socket lock.
> diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
> index 9049d26486460..bb7d81a95baa6 100644
> --- a/net/vmw_vsock/vsock_bpf.c
> +++ b/net/vmw_vsock/vsock_bpf.c
> @@ -50,7 +50,9 @@ static bool vsock_msg_wait_data(struct sock *sk, struct sk_psock *psock, long ti
> sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> ret = vsock_has_data(sk, psock);
> if (!ret) {
> + release_sock(sk);
> wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
> + lock_sock(sk);
> ret = vsock_has_data(sk, psock);
[Severity: Medium]
With the lock dropped here, virtio_transport_recv_pkt() can run while the
reader sleeps. Can it wake the reader for events that carry no data?
The reader sleeps on sk_sleep(sk) with woken_wake_function(), which
ignores the poll key. virtio_transport_recv_pkt() calls sk_write_space()
for every packet when space is available:
net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_pkt() {
...
if (space_available)
sk->sk_write_space(sk);
...
}
OP_CREDIT_UPDATE in virtio_transport_recv_connected() does the same. For
vsock this is sock_def_write_space(), which calls
wake_up_interruptible_sync_poll() on the same wait queue.
OP_SHUTDOWN and RST call sk_state_change(). An SK_DROP verdict or a
redirect to another socket also wakes the reader through
sk_psock_data_ready(), without queuing anything for it.
After such a wakeup, vsock_has_data() returns false. vsock_msg_wait_data()
then returns false without waiting again for the rest of timeo, and
vsock_bpf_recvmsg() does this:
if (!vsock_msg_wait_data(sk, psock, timeo)) {
copied = -EAGAIN;
break;
}
Would a blocking recv() with no SO_RCVTIMEO then fail with -EAGAIN as
soon as the peer sends a credit update?
For comparison, vsock_connectible_wait_data() loops until one of these
happens: data arrives, sk_err is set, RCV_SHUTDOWN or peer SEND_SHUTDOWN
is seen, the timeout expires, or a signal is pending. On a signal it
returns sock_intr_errno().
The BPF path also returns -EAGAIN when a signal is pending. That part
existed before this patch.
Should vsock_msg_wait_data() loop the same way that
vsock_connectible_wait_data() does?
> }
> sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can a
blocking recv() still hang after the peer shuts down or resets?
The only exit condition vsock_msg_wait_data() checks is a local shutdown:
if (sk->sk_shutdown & RCV_SHUTDOWN)
return true;
The virtio, vhost and loopback transports report a peer shutdown only
through vsk->peer_shutdown:
- virtio_transport_recv_connected() sets it for OP_SHUTDOWN.
- virtio_transport_do_close() sets peer_shutdown = SHUTDOWN_MASK and
SOCK_DONE for RST or a full close.
sk->sk_shutdown changes only on a local shutdown().
Suppose the peer has shut down and the receive and psock queues are
empty:
- vsock_bpf_recvmsg() skips the native fallback.
- sk_msg_recvmsg() returns 0.
- vsock_msg_wait_data() sleeps in wait_woken().
vsock_remove_sock() has already removed the socket, so no further wakeups
will arrive. The recv() sleeps until a signal arrives or a finite
SO_RCVTIMEO expires.
Meanwhile vsock_poll() reports EPOLLIN | EPOLLRDNORM on peer
SEND_SHUTDOWN. poll() says the socket is readable while recv() blocks.
If the reader is already asleep when the shutdown arrives, the
sk_state_change() wakeup that this patch now allows makes recv() return
-EAGAIN instead of 0. A retry then hangs as described above.
The native vsock_connectible_wait_data() handles this case:
if (sk->sk_err != 0 ||
(sk->sk_shutdown & RCV_SHUTDOWN) ||
(vsk->peer_shutdown & SEND_SHUTDOWN)) {
break;
}
__vsock_connectible_recvmsg() also returns 0 for a socket that is not
TCP_ESTABLISHED and has SOCK_DONE set.
After this fix, this is the remaining case where a blocking recv() hangs
until a signal arrives. Could it be handled while aligning with
vsock_connectible_wait_data()?
[Severity: High]
This is also a pre-existing issue and was not introduced by this patch.
Can vsock_bpf_recvmsg() spin forever with the socket lock held when len
is 0?
tcp_bpf, udp_bpf and unix_bpf all return early with "if (!len) return 0;",
and __vsock_connectible_recvmsg() handles len == 0 explicitly.
vsock_bpf_recvmsg() does neither.
If psock->ingress_msg is not empty, for example after SK_PASS or an
ingress redirect, sk_psock_queue_empty() is false. The native fallback is
then skipped:
if (vsock_has_data(sk, psock) && sk_psock_queue_empty(psock)) {
With len == 0, __sk_msg_recvmsg() never enters its
"while (copied != len)" loop. It returns 0 without consuming anything, so
the loop in vsock_bpf_recvmsg() repeats:
copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
while (copied == 0) {
long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
if (!vsock_msg_wait_data(sk, psock, timeo)) {
copied = -EAGAIN;
break;
}
...
copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
}
vsock_msg_wait_data() sees that vsock_has_data() is true and returns true
without sleeping, so the loop never reaches the new release_sock(). The
loop never calls cond_resched() or checks for signals, and it keeps
ownership of lock_sock(sk) the whole time.
Would the vsock-loopback worker or the vhost/virtio rx path then block in
lock_sock() on this socket? That would stop rx for every socket on the
transport, which is the symptom this commit sets out to fix.
Would adding the same "if (!len) return 0;" check that the other *_bpf
recvmsg implementations have be enough here?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929-kbh3-1-022-fix-v1-1-cc97cc95d269%40eldare.de
^ permalink raw reply [flat|nested] 3+ messages in thread