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

[netdev-bot+sashiko@kernel.org]

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