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

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

[Jerome Mohm via B4 Relay]

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;
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);
+
 	psock = sk_psock_get(sk);
 	if (unlikely(!psock))
 		return __vsock_recvmsg(sk, msg, len, flags);
@@ -102,10 +116,9 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
 	while (copied == 0) {
 		long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
 
-		if (!vsock_msg_wait_data(sk, psock, timeo)) {
-			copied = -EAGAIN;
+		copied = vsock_msg_wait_data(sk, psock, timeo);
+		if (copied)
 			break;
-		}
 
 		if (sk_psock_queue_empty(psock)) {
 			release_sock(sk);

---
base-commit: 6d2cf411aba2aa0bd641b49be5b1964ac5097a50
change-id: 20261001-kbh3-1-022-fix-v2-e3f14290641a

Best regards,
--  
Jerome Mohm <jrmmhm.kernel@eldare.de>

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 2

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] vsock_msg_wait_data() now drops the socket lock around
  wait_woken().
- [Low] In vsock_bpf_recvmsg(), a reader asleep during a transport reset
  gets -ENOTCONN, or 0 if SOCK_DONE is set.
- [Low] vsock_msg_wait_data() (net/vmw_vsock/vsock_bpf.c:54-66) checks
  `if (!timeo)` before `signal_pending(current)`.

--

vsock/bpf: release sock lock while waiting for data in recvmsg

This patch drops the socket lock around wait_woken() in
vsock_msg_wait_data(), so the virtio rx worker no longer deadlocks
against a sockmap reader. The wait now loops until one of these
happens: data arrives, the timeout expires, a signal is pending, or
vsock_rx_done() reports a state in which __vsock_recvmsg() would not
wait. Those states, and zero-length, MSG_OOB and MSG_ERRQUEUE reads,
are handed to __vsock_recvmsg().

> diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
> index 9049d26486460..c0c9e69860dca 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;
> -
> -	DEFINE_WAIT_FUNC(wait, woken_wake_function);
> +	return sk->sk_err || sk->sk_state != TCP_ESTABLISHED ||
> +	       (sk->sk_shutdown & RCV_SHUTDOWN) ||
> +	       (vsock_sk(sk)->peer_shutdown & SEND_SHUTDOWN);
> +}

[Severity: Low]
Does this comment hold for the sk_err case after a transport reset?

virtio_vsock_reset_sock() sets sk_state = TCP_CLOSE and
sk_err = ECONNRESET, then calls sk_error_report(). A reader asleep in
vsock_msg_wait_data() wakes up and finds no data. vsock_rx_done() is
true, so it leaves the loop with ret = 0. vsock_bpf_recvmsg() then
sees sk_psock_queue_empty() and calls __vsock_recvmsg().

__vsock_connectible_recvmsg() checks the state first:

net/vmw_vsock/af_vsock.c:__vsock_connectible_recvmsg() {
    ...
	if (!transport || sk->sk_state != TCP_ESTABLISHED) {
		...
		if (sock_flag(sk, SOCK_DONE))
			err = 0;
		else
			err = -ENOTCONN;

		goto out;
	}
    ...
}

So the sk_err conversion in __vsock_stream_recvmsg() is never reached:

	if (sk->sk_err)
		err = -sk->sk_err;

A native reader that is already waiting in
vsock_connectible_wait_data() has passed the state check. It returns
-ECONNRESET on SOCK_STREAM and 0 on SOCK_SEQPACKET.

The commit message already notes this ("a reader asleep at a virtio
transport reset gets -ENOTCONN, where a native socket gets -ECONNRESET
(0 on seqpacket)"). It is also better than the -EAGAIN this reader got
before the patch.

The wait loop stops on sk_err, but the code it hands off to can't
report that error. Could the wait path return the pending error the
way the native path does? If not, could the comment be reworded to
cover this case?

[ ... ]

> +	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;
> +		}

[Severity: Low]
What happens when the timeout and a signal land in the same wakeup?

Say wait_woken() returns 0 because SO_RCVTIMEO ran out, and a signal
is pending at the same time. The next iteration hits the !timeo test
first and returns -EAGAIN. The signal check never runs.

The native vsock_connectible_wait_data() checks for a signal first
after sleeping:

	if (signal_pending(current)) {
		err = sock_intr_errno(timeout);
		break;
	} else if (timeout == 0) {
		err = -EAGAIN;
		break;
	}

That returns -EINTR for a finite timeout. The patch notes list this
case under "Not tested" ("returns -EAGAIN, native -EINTR").

Simply swapping the two checks at the top of the loop would make a
non-blocking read with a pending signal return -EINTR. Would checking
signal_pending() right after wait_woken() returns, as the native loop
does, make this match?

> +		release_sock(sk);
> +		timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
> +		lock_sock(sk);
>  	}

[Severity: Medium]
Now that the lock is dropped around wait_woken(), can vsk->transport
be NULL when the loop condition is checked again?

The WARN_ON_ONCE(!vsk->transport) check in vsock_bpf_recvmsg() runs
only once, at entry. After lock_sock(), the loop condition evaluates
vsock_has_data()->vsock_connectible_has_data() before
vsock_rx_done(), and that function has:

	if (WARN_ON(!vsk->transport))
		return 0;

While the reader is unlocked, vsock_connect()->vsock_assign_transport()
can release the old transport, deassign it, and then return early
without assigning a new one:

	vsk->transport->release(vsk);
	vsock_deassign_transport(vsk);
	...
	if (!new_transport->seqpacket_allow ||
	    !new_transport->seqpacket_allow(vsk, remote_cid)) {
		module_put(new_transport->module);
		return -ESOCKTNOSUPPORT;
	}
	...
	ret = new_transport->init(vsk, psk);
	if (ret) {
		module_put(new_transport->module);
		return ret;
	}

One possible sequence:

reader (sockmap socket)          connect thread
vsock_bpf_recvmsg()
  vsock_msg_wait_data()
    release_sock()
    wait_woken()
                                 transport reset: TCP_CLOSE, sk_err set
                                 earlier vsock_connect() retakes the
                                   lock, sees sk_err, sets
                                   SS_UNCONNECTED
                                 connect() again to a CID on another
                                   transport
                                   vsock_assign_transport()
                                     vsock_deassign_transport()
                                     seqpacket_allow() or init() fails
    lock_sock()
    vsock_has_data()
      vsock_connectible_has_data()
        WARN_ON(!vsk->transport)

The visible effect is a WARN splat, or a panic with panic_on_warn.
vsock_connectible_has_data() returns 0, vsock_rx_done() then returns
true, and __vsock_connectible_recvmsg() handles a NULL transport. The
native vsock_connectible_wait_data() checks in the same order.

Before this patch, the lock was held for the whole wait on the BPF
path, so this interleaving could not happen there. Would checking
vsock_rx_done() or !vsk->transport before vsock_has_data() in the loop
condition avoid the warning?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109-kbh3-1-022-fix-v2-v5-1-4d9af04f0fad%40eldare.de