Re: [PATCH net v5] vsock/bpf: release sock lock while waiting for data in recvmsg

[Stefano Garzarella <sgarzare@redhat.com>]

On Mon, Nov 09, 2026 at 09:08:29PM +0100, Jerome Mohm via B4 Relay 
wrote:
>From: Jerome Mohm <jrmmhm.kernel@eldare.de>
>
>vsock_bpf_recvmsg() sleeps in vsock_msg_wait_data() with the socket lock
>held, so the virtio delivery worker, which needs that lock, deadlocks
>with the reader and all vsock rx on that worker stops.
>
>Release the lock around the wait. The reader can then wake without
>data, so the wait loops until data arrives, the timeout, a signal
>(wait_woken() returns at once while one is pending), or a state in
>which __vsock_recvmsg() would not wait. Those states, and zero-length,
>MSG_OOB and MSG_ERRQUEUE reads, go to __vsock_recvmsg(), as Alexei
>suggested; a non-blocking read now sees them before -EAGAIN. The loop
>and the zero-length check belong to the fix (without them the reader
>returns early or spins with the lock held), so this is one patch;

I don't understand why that fix can't be in a fist patch of the 
series...

>MSG_OOB and MSG_ERRQUEUE just share that path. One tested difference
>remains: a reader asleep at a virtio transport reset gets -ENOTCONN,
>where a native socket gets -ECONNRESET (0 on seqpacket).
>
>The deadlock and the MSG_ERRQUEUE case were found with the help of
>Anthropic's Claude, the rest by the netdev Sashiko reviews and Alexei.
>Each was reproduced in a VM without kernel changes or error injection
>(the resets unbind the guest's virtio-vsock device), except one Sashiko
>case that was checked in the code; with this patch they behave as on a
>native socket apart from the reset case above. The vsock vmtest.sh
>single-VM tests and the BPF sockmap selftests still pass, though they
>never sleep in this wait.
>
>Fixes: 634f1a7110b4 ("vsock: support sockmap")
>Suggested-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
>Cc: stable@vger.kernel.org
>Signed-off-by: Jerome Mohm <jrmmhm.kernel@eldare.de>
>---
>Changes in v5:
>- One patch, using Alexei's approach: in the states where
>  __vsock_recvmsg() does not wait, and for zero-length, MSG_OOB and
>  MSG_ERRQUEUE reads, fall back to __vsock_recvmsg() instead of
>  copying its return values (Alexei).
>- So after a reset, recv(), a zero-length read and MSG_OOB return
>  -ENOTCONN, and a peer shutdown with SO_INQ reports SCM_INQ, as on a
>  socket outside a sockmap. A reset while the reader sleeps now
>  returns -ENOTCONN instead of -ECONNRESET.
>- This also covers the netdev Sashiko review of v4: SEQPACKET after a
>  reset, MSG_OOB on a closed socket and SCM_INQ on a zero-length read;
>  SHUT_RD with a pending sk_err by code reading only. A SEQPACKET
>  reader asleep at a reset still gets -ENOTCONN (native 0).
>- Don't cite the selftests as coverage of the wait (Alexei).
>- Link to v4: https://patch.msgid.link/20261106-kbh3-1-022-fix-v2-v4-0-c7d36703814d@eldare.de
>
>Not tested: virtio-mmio, repeated runs, a sockmap delete while a
>reader sleeps, a signal arriving in the same wakeup as the timeout
>(returns -EAGAIN, native -EINTR), and the vmtest.sh ns_* tests.
>Still left for separate patches, as in v4: data that sits only in
>psock->ingress_skb, and READ_ONCE()/WRITE_ONCE() for the lockless
>accesses.
>
>Changes in v4: comments, one data <= 0 check and message changes
>(Stefano); rebase; retest with a new dataless-wakeup reproducer.
>Changes in v3: split into two patches (Michal); end the wait after a
>reset read with SO_ERROR, and MSG_OOB returns -EOPNOTSUPP (Sashiko).
>Changes in v2: loop in the wait, len == 0 guard (Sashiko); MSG_ERRQUEUE
>to the native path.
>---
> net/vmw_vsock/vsock_bpf.c | 43 ++++++++++++++++++++++++++++---------------
> 1 file changed, 28 insertions(+), 15 deletions(-)
>
>diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
>index 9049d2648646..c0c9e69860dc 100644
>--- a/net/vmw_vsock/vsock_bpf.c
>+++ b/net/vmw_vsock/vsock_bpf.c
>@@ -34,24 +34,35 @@ static bool vsock_has_data(struct sock *sk, struct sk_psock *psock)
> 	return vsock_sk_has_data(sk, psock);
> }
>
>-static bool vsock_msg_wait_data(struct sock *sk, struct sk_psock *psock, long timeo)
>+/* __vsock_recvmsg() doesn't wait in these states and knows what to return. */
>+static bool vsock_rx_done(struct sock *sk)
> {
>-	bool ret;
>+	return sk->sk_err || sk->sk_state != TCP_ESTABLISHED ||
>+	       (sk->sk_shutdown & RCV_SHUTDOWN) ||
>+	       (vsock_sk(sk)->peer_shutdown & SEND_SHUTDOWN);
>+}
>+
>+static int vsock_msg_wait_data(struct sock *sk, struct sk_psock *psock,
>+			       long timeo)
>+{
>+	int ret = 0;
>
> 	DEFINE_WAIT_FUNC(wait, woken_wake_function);
>
>-	if (sk->sk_shutdown & RCV_SHUTDOWN)
>-		return true;
>-
>-	if (!timeo)
>-		return false;
>-
> 	add_wait_queue(sk_sleep(sk), &wait);
> 	sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk);
>-	ret = vsock_has_data(sk, psock);
>-	if (!ret) {
>-		wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
>-		ret = vsock_has_data(sk, psock);
>+	while (!vsock_has_data(sk, psock) && !vsock_rx_done(sk)) {
>+		if (!timeo) {
>+			ret = -EAGAIN;
>+			break;
>+		}
>+		if (signal_pending(current)) {
>+			ret = sock_intr_errno(timeo);
>+			break;
>+		}
>+		release_sock(sk);
>+		timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
>+		lock_sock(sk);
> 	}
> 	sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
> 	remove_wait_queue(sk_sleep(sk), &wait);
>@@ -80,6 +91,9 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
> 	struct vsock_sock *vsk;
> 	int copied;
>
>+	if (unlikely(!len || (flags & (MSG_OOB | MSG_ERRQUEUE))))
>+		return __vsock_recvmsg(sk, msg, len, flags);

... I mean, why this change ^^^ can't be in patch 1/2?

BTW I'm not asking to split it (at least for now).

I saw some comments from sashiko, can you reply to them?
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109-kbh3-1-022-fix-v2-v5-1-4d9af04f0fad%40eldare.de

Thanks,
Stefano