Re: [PATCH net v5] 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 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