Re: [PATCH net v4 1/2] 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 1 potential
issue(s) to consider.

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

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 commit message says that with the series applied the
  reproduced cases 'behave as on a native socket, except for the reset…

--

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

This patch fixes a deadlock in vsock_bpf_recvmsg(). The function held
lock_sock() while it slept in wait_woken(), and the transport needs that
lock to deliver data. The lock is now released around wait_woken(), and
the wait loops until data arrives or one of the native exit conditions
applies (socket error, shutdown, not connected, timeout or signal).

> diff --git a/net/vmw_vsock/vsock_bpf.c b/net/vmw_vsock/vsock_bpf.c
> index 9049d26486460..6d96ff03f28c4 100644
> --- a/net/vmw_vsock/vsock_bpf.c
> +++ b/net/vmw_vsock/vsock_bpf.c

[ ... ]

> +	while (1) {
> +		if (vsock_has_data(sk, psock)) {
> +			ret = 1;
> +			break;
> +		}
> +
> +		/* Stop on a socket error, as vsock_connectible_wait_data()
> +		 * does, so that recv() returns it; the checks below would
> +		 * end the wait with 0 or -ENOTCONN instead.
> +		 */
> +		if (sk->sk_err) {
> +			ret = -sk->sk_err;
> +			break;
> +		}

[Severity: Medium]
Does this match the native path for SOCK_SEQPACKET as well as
SOCK_STREAM?

vsock_connectible_wait_data() stops on sk_err, but it returns 0 rather
than the error:

net/vmw_vsock/af_vsock.c:vsock_connectible_wait_data() {
    ...
		if (sk->sk_err != 0 ||
		    (sk->sk_shutdown & RCV_SHUTDOWN) ||
		    (vsk->peer_shutdown & SEND_SHUTDOWN)) {
			break;
		}
    ...
}

Only __vsock_stream_recvmsg() converts that into -sk->sk_err afterwards.
__vsock_seqpacket_recvmsg() returns the 0 directly:

	err = vsock_connectible_wait_data(sk, &wait, timeout, NULL, 0);
	if (err <= 0)
		goto out;

With this patch, vsock_bpf_recvmsg() passes the return value through
unchanged (copied = data). sock_map accepts vsock SOCK_SEQPACKET sockets.

Take a SEQPACKET socket in a sockmap with a reader blocked in recv(). If
virtio_vsock_reset_sock() runs and sets sk_err = ECONNRESET, the reader
gets -ECONNRESET here, while a native socket returns 0. The same applies
to other sk_err values, such as EPIPE from sk_psock_report_error().

The check order also differs from __vsock_connectible_recvmsg(). That
function checks the state and transport first, then returns 0 on a local
RCV_SHUTDOWN before it waits:

	if (sk->sk_shutdown & RCV_SHUTDOWN) {
		err = 0;
		goto out;
	}

This loop checks sk_err before RCV_SHUTDOWN. So a socket that is still
TCP_ESTABLISHED, has done a local SHUT_RD and has a pending sk_err would
return -sk_err from the BPF path but 0 natively.

The commit message says:

    With the series applied they behave as on
    a native socket, except for the reset case above

and the code comment claims parity with vsock_connectible_wait_data().
Neither holds for SEQPACKET, or for the SHUT_RD plus sk_err case.

Returning the error may be the better behaviour. If so, could the commit
message and comment say so? The alternative is to follow the native
semantics for each socket type. The later patch in the series ("vsock/bpf:
fix zero-length, MSG_OOB and MSG_ERRQUEUE reads in recvmsg") does not
change vsock_msg_wait_data(), so the difference is still there at the end
of the series.

> +
> +		if ((sk->sk_shutdown & RCV_SHUTDOWN) ||
> +		    (vsk->peer_shutdown & SEND_SHUTDOWN)) {
> +			ret = 0;
> +			break;
> +		}

[ ... ]

> @@ -101,9 +140,10 @@ 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;
>  		}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-kbh3-1-022-fix-v2-v4-0-c7d36703814d%40eldare.de