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

[PATCH net v2] 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() takes lock_sock(sk) and holds it across the receive
loop, including vsock_msg_wait_data(), which slept in wait_woken() without
dropping the lock. When data arrived the transport's delivery context (the
vsock-loopback worker, or the virtio/vhost rx path) called
virtio_transport_recv_pkt() -> lock_sock() on the same socket and blocked,
so neither side made progress and a blocking recv() hung; the hung-task
watchdog reported the delivery worker in D state.

Dropping the socket lock around the wait is necessary but not sufficient:
vsock_msg_wait_data() also did not loop and checked too few conditions,
which left three further problems in the same helper.

 - It did not loop. Once the lock is dropped, a wakeup that is not for new
   data (for example a credit update via sk_write_space()) made the helper
   return with nothing queued, so a blocking recv() returned -EAGAIN
   prematurely instead of waiting for data.

 - It checked only sk->sk_shutdown & RCV_SHUTDOWN, not sk->sk_err or
   vsk->peer_shutdown & SEND_SHUTDOWN. Once the peer shut down for send or
   the socket errored (for example a reset), recv() did not notice and kept
   waiting instead of returning 0 or the error.

 - vsock_bpf_recvmsg() had no "if (!len) return 0;" guard, so a
   zero-length recv() with data queued never satisfied the loop's
   copied == 0 exit and spun holding lock_sock(), which wedges the delivery
   worker and stalls all rx on the transport.

Rewrite vsock_msg_wait_data() to loop: return the data when it is ready, 0
on RCV_SHUTDOWN or peer SEND_SHUTDOWN, the negative socket error on sk_err,
-EAGAIN on timeout and the signal error on a pending signal, releasing the
socket lock around the wait and re-acquiring it before re-checking. Add the
len == 0 guard to vsock_bpf_recvmsg(), and route MSG_ERRQUEUE to the native
path before that guard so a zero-length error-queue read is not swallowed,
as tcp_bpf does. The exit conditions follow vsock_connectible_wait_data();
dropping the lock across the wait matches unix_bpf (u->iolock) and tcp_bpf
(sk_wait_event()).

Testing: built a fuzz kernel (KASAN + lockdep) on the net tree
(v7.3-rc4) and ran reproducers over the loopback transport on a private
VM, each before and after the fix. Before: a blocking recv() on a
sockmap socket deadlocks the vsock delivery worker (hung-task); and with
only the lock dropped, a spurious credit-update wakeup returns -EAGAIN,
a peer SEND_SHUTDOWN returns -EAGAIN instead of 0, a zero-length recv()
with queued data never returns and wedges the delivery worker (hung-
task), and a len == 0 MSG_ERRQUEUE read returns 0 instead of reaching
the error queue. After: recv() returns the data, ignores the spurious
wakeup and returns the real byte, returns 0 on peer shutdown, returns 0
for len == 0, and routes MSG_ERRQUEUE to the error-queue handler; no
KASAN or lockdep report.

Fixes: 634f1a7110b4 ("vsock: support sockmap")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Jerome Mohm <jrmmhm.kernel@eldare.de>
---
Changes since v1 [1] (netdev review by the Sashiko bot):
- v1 only released the socket lock around the wait. v2 additionally makes
  vsock_msg_wait_data() loop (no premature -EAGAIN on a dataless wakeup),
  checks sk_err and the peer SEND_SHUTDOWN as well as RCV_SHUTDOWN (correct
  EOF/error on peer shutdown or reset), and adds the len == 0 guard to
  vsock_bpf_recvmsg() plus an MSG_ERRQUEUE route ahead of it (no spin on a
  zero-length recv; an error-queue read is not swallowed).
- Garzarella's Acked-by on v1 is intentionally dropped: v2 is a materially
  larger change and needs a fresh review.

[1] https://lore.kernel.org/netdev/20260929-kbh3-1-022-fix-v1-1-cc97cc95d269@eldare.de/
---
 net/vmw_vsock/vsock_bpf.c | 59 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 14 deletions(-)

diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
index 9049d2648646..127a20429aa6 100644
--- a/net/vmw_vsock/vsock_bpf.c
+++ b/net/vmw_vsock/vsock_bpf.c
@@ -34,24 +34,46 @@ 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)
+/* Returns 1 if data is ready, 0 on EOF/shutdown, or a negative error. */
+static int vsock_msg_wait_data(struct sock *sk, struct sk_psock *psock, long timeo)
 {
-	bool ret;
+	struct vsock_sock *vsk = vsock_sk(sk);
+	int ret;
 
 	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 (1) {
+		if (vsock_has_data(sk, psock)) {
+			ret = 1;
+			break;
+		}
+
+		if (sk->sk_err) {
+			ret = -sk->sk_err;
+			break;
+		}
+
+		if ((sk->sk_shutdown & RCV_SHUTDOWN) ||
+		    (vsk->peer_shutdown & SEND_SHUTDOWN)) {
+			ret = 0;
+			break;
+		}
+
+		if (!timeo) {
+			ret = -EAGAIN;
+			break;
+		}
+
+		release_sock(sk);
+		timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
+		lock_sock(sk);
+
+		if (signal_pending(current)) {
+			ret = sock_intr_errno(timeo);
+			break;
+		}
 	}
 	sk_clear_bit(SOCKWQ_ASYNC_WAITDATA, sk);
 	remove_wait_queue(sk_sleep(sk), &wait);
@@ -80,6 +102,12 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
 	struct vsock_sock *vsk;
 	int copied;
 
+	if (unlikely(flags & MSG_ERRQUEUE))
+		return __vsock_recvmsg(sk, msg, len, flags);
+
+	if (!len)
+		return 0;
+
 	psock = sk_psock_get(sk);
 	if (unlikely(!psock))
 		return __vsock_recvmsg(sk, msg, len, flags);
@@ -101,11 +129,14 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
 	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
 	while (copied == 0) {
 		long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
+		int data = vsock_msg_wait_data(sk, psock, timeo);
 
-		if (!vsock_msg_wait_data(sk, psock, timeo)) {
-			copied = -EAGAIN;
+		if (data < 0) {
+			copied = data;
 			break;
 		}
+		if (!data)
+			break;
 
 		if (sk_psock_queue_empty(psock)) {
 			release_sock(sk);

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

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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1

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] The rewritten vsock_msg_wait_data()
  (net/vmw_vsock/vsock_bpf.c:47-77) detects a reset only through…
- [Low] vsock_bpf_recvmsg() (net/vmw_vsock/vsock_bpf.c:105-109) sends
  only MSG_ERRQUEUE to the native path before the new `if (!len) return…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  rewrites the code around it.

--

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

This patch changes vsock_msg_wait_data() so it drops the socket lock
around wait_woken(). It also makes the helper loop until data is ready,
an error or shutdown is seen, the timeout expires or a signal is pending.
In vsock_bpf_recvmsg(), MSG_ERRQUEUE now goes to the native path, and
zero-length reads return 0 early.

> diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
> index 9049d26486460..127a20429aa6c 100644
> --- a/net/vmw_vsock/vsock_bpf.c
> +++ b/net/vmw_vsock/vsock_bpf.c
> @@ -34,24 +34,46 @@ 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)
> +/* Returns 1 if data is ready, 0 on EOF/shutdown, or a negative error. */
> +static int vsock_msg_wait_data(struct sock *sk, struct sk_psock *psock, long timeo)
>  {

[ ... ]

> +	while (1) {
> +		if (vsock_has_data(sk, psock)) {
> +			ret = 1;
> +			break;
> +		}
> +
> +		if (sk->sk_err) {
> +			ret = -sk->sk_err;
> +			break;
> +		}
> +
> +		if ((sk->sk_shutdown & RCV_SHUTDOWN) ||
> +		    (vsk->peer_shutdown & SEND_SHUTDOWN)) {
> +			ret = 0;
> +			break;
> +		}

[Severity: Medium]
Can this loop sleep forever on a connection that has already been reset?

The only reset signal checked here is sk->sk_err. Its value can be cleared
from userspace. virtio_vsock_reset_sock() runs on
VIRTIO_VSOCK_EVENT_TRANSPORT_RESET (for example after live migration) and
on device removal. It sets only the state and the error:

net/vmw_vsock/virtio_transport.c:virtio_vsock_reset_sock() {
	...
	sk->sk_state = TCP_CLOSE;
	sk->sk_err = ECONNRESET;
	sk_error_report(sk);
}

It does not set any sk_shutdown or peer_shutdown bits, and the psock stays
in place.

Suppose the application then calls getsockopt(SO_ERROR). sk_getsockopt()
calls sock_error(), which clears sk_err with xchg(). The next recv() on the
sockmap'd socket with empty queues gets copied == 0 from sk_msg_recvmsg()
and enters this loop. None of the exit conditions is true there. A blocking
call waits until a signal arrives or SO_RCVTIMEO expires, and a
non-blocking call returns -EAGAIN.

The native path handles this case before it ever waits:

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

The commit message says this fixes the case where "the socket errored (for
example a reset), recv() did not notice and kept waiting". Should the loop
also check sk->sk_state and SOCK_DONE, so that case still works after the
error has been read?

> +
> +		if (!timeo) {
> +			ret = -EAGAIN;
> +			break;
> +		}
> +
> +		release_sock(sk);
> +		timeo = wait_woken(&wait, TASK_INTERRUPTIBLE, timeo);
> +		lock_sock(sk);

[ ... ]

> @@ -80,6 +102,12 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
>  	struct vsock_sock *vsk;
>  	int copied;
>  
> +	if (unlikely(flags & MSG_ERRQUEUE))
> +		return __vsock_recvmsg(sk, msg, len, flags);
> +
> +	if (!len)
> +		return 0;

[Severity: Low]
Should MSG_OOB be rejected here as well, next to MSG_ERRQUEUE?

With this shortcut, recv(fd, buf, 0, MSG_OOB) on a sockmap'd vsock socket
always returns 0.

Before this patch, the same call reached __vsock_recvmsg() when transport
data was pending and ingress_msg was empty. __vsock_connectible_recvmsg()
then returned -EOPNOTSUPP before its own zero-length check:

	if (flags & MSG_OOB) {
		err = -EOPNOTSUPP;
		goto out;
	}

The commit message cites unix_bpf_recvmsg() as a model, and it does this
before its len == 0 guard:

	if (flags & MSG_OOB)
		return -EOPNOTSUPP;

	if (!len)
		return 0;

There is also an older issue from 634f1a7110b4. With len > 0 and data in
ingress_msg, a MSG_OOB read reaches sk_msg_recvmsg(). That function only
looks at MSG_PEEK, so it copies and consumes ordinary stream bytes. With
empty queues, the read blocks in the wait loop instead of failing right
away. Rejecting MSG_OOB next to the new MSG_ERRQUEUE check would fix both
cases.

> +
>  	psock = sk_psock_get(sk);
>  	if (unlikely(!psock))
>  		return __vsock_recvmsg(sk, msg, len, flags);

[ ... ]

> @@ -101,11 +129,14 @@ static int vsock_bpf_recvmsg(struct sock *sk, struct msghdr *msg,
>  	copied = sk_msg_recvmsg(sk, psock, msg, len, flags);
>  	while (copied == 0) {
>  		long timeo = sock_rcvtimeo(sk, flags & MSG_DONTWAIT);
> +		int data = vsock_msg_wait_data(sk, psock, timeo);
>  
> -		if (!vsock_msg_wait_data(sk, psock, timeo)) {
> -			copied = -EAGAIN;
> +		if (data < 0) {
> +			copied = data;
>  			break;
>  		}
> +		if (!data)
> +			break;
>  
>  		if (sk_psock_queue_empty(psock)) {
>  			release_sock(sk);

[Severity: High]
This isn't a bug introduced by this patch, but the patch rewrites the code
around it. Can a blocking recv() hang here while data is waiting in
psock->ingress_msg?

vsock_msg_wait_data() returns 1 whenever vsock_has_data() is true.
vsock_sk_has_data() counts a non-empty psock->ingress_skb as data:

#define vsock_sk_has_data(__sk, __psock)				\
		({	!skb_queue_empty(&(__sk)->sk_receive_queue) ||	\
			!skb_queue_empty(&(__psock)->ingress_skb) ||	\
			!list_empty(&(__psock)->ingress_msg);		\
		})

sk_psock_queue_empty(), however, only checks ingress_msg. The same pair of
checks is also used before the loop:

	if (vsock_has_data(sk, psock) && sk_psock_queue_empty(psock)) {

Redirected-ingress data always passes through ingress_skb first:

net/core/skmsg.c:sk_psock_skb_redirect() {
	...
	skb_queue_tail(&psock_other->ingress_skb, skb);
	schedule_delayed_work(&psock_other->work, 0);
	...
}

If the only pending data is on ingress_skb and sk_psock_backlog() has not
run yet, the reader drops the lock and the psock reference here and falls
into the native path:

__vsock_recvmsg()
  __vsock_connectible_recvmsg()
    __vsock_stream_recvmsg()
      vsock_connectible_wait_data()

That native wait loop only wakes for vsock_connectible_has_data() != 0,
sk_err or a shutdown flag. It never looks at the psock queues. When the
backlog worker later moves the data to ingress_msg and wakes the socket,
the native reader sees transport data == 0 and goes back to sleep.

If the socket also has a verdict program, sk_psock_verdict_data_ready()
pulls new transport data via read_skb, and an SK_PASS verdict queues it on
ingress_msg. The transport count therefore stays at 0. The blocking recv()
then waits until a signal arrives or SO_RCVTIMEO expires.

The reader can enter this window on a fresh recv(). It can also enter it
after a spurious wakeup, such as the credit-update sk_write_space() wakeup
mentioned in the commit message, which now makes the loop re-check.

Should this fallback also take psock->ingress_skb into account?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001-kbh3-1-022-fix-v2-v1-1-cfae958a1976%40eldare.de