Re: [PATCH net v3 5/8] vsock: treat TCP_CLOSING as once-established

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] The failed-connect cleanup at the out_reset label of
  vsock_connect() (net/vmw_vsock/af_vsock.c:1883-1884), which this patch…
- [Low] Concern (validated as NOT a defect to report against this
  series): the new short-circuit in vsock_connect() enumerates only…

--

vsock: treat TCP_CLOSING as once-established

The patch makes blocking connect() stop treating a socket that already
reached TCP_ESTABLISHED and then moved to TCP_CLOSING (or picked up an
sk_err) as a failed connection attempt, returning 0 instead. It also makes
the out_reset cleanup call vsock_unconnected_reset() unconditionally rather
than only when err is non-zero.

Link: https://lore.kernel.org/netdev/anzT1fREOSyHT99k@v4bel/

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 5cecd1c9c6e50..ff4140aaf1f32 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1846,15 +1846,19 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  		timeout = schedule_timeout(timeout);
>  		lock_sock(sk);
>  
> -		/* Connection established. Whatever happens to socket once we
> -		 * release it, that's not connect()'s concern. No need to go
> +		/* Connection was established. Whatever happens to socket once
> +		 * we release it, that's not connect()'s concern. No need to go
>  		 * into signal and timeout handling. Call it a day.
>  		 *
>  		 * Note that allowing to "reset" an already established socket
>  		 * here is racy and insecure.
>  		 */
> -		if (sk->sk_state == TCP_ESTABLISHED)
> -			break;
> +		if (sk->sk_state == TCP_ESTABLISHED ||
> +		    sk->sk_state == TCP_CLOSING) {
> +			err = 0;
> +			finish_wait(sk_sleep(sk), &wait);
> +			goto out;
> +		}

[Severity: Low]
This isn't a bug, but the new short circuit lists only TCP_ESTABLISHED and
TCP_CLOSING as "once established", while an established socket can also be
taken straight to TCP_CLOSE by a transport event while connect() is
scheduled out:

net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
	...
			sk->sk_state = TCP_CLOSE;
		}
		sk->sk_state_change(sk);
}

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

Those sockets were established as well, but they miss this check, so
connect() still returns -ECONNRESET or -ETIMEDOUT for them.

The cleanup side of that case is covered by the follow-up patch in this
series, "vsock: handle socket bricking due to transport event", which adds
an early return to vsock_unconnected_reset() for sock->state ==
SS_CONNECTED or SS_DISCONNECTING, and neither of the two transport paths
above modifies sk->sk_socket->state.

Is the remaining difference in the connect() return value for the TCP_CLOSE
case the intended behaviour?

>  
>  		/* If connection was _not_ established and a signal/timeout came
>  		 * to be, we want the socket's state reset. We keep the binding.
> @@ -1877,8 +1881,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  out_wait:
>  	finish_wait(sk_sleep(sk), &wait);
>  out_reset:
> -	if (err)
> -		vsock_unconnected_reset(sk);
> +	vsock_unconnected_reset(sk);

[Severity: High]
With this call now unconditional, can a VMCI detach during the handshake
turn this cleanup into a silent no-op plus a warning backtrace?

vmci_transport_handle_detach() sets the flags before it inspects sk_state,
and it explicitly handles the TCP_SYN_SENT case:

net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
	...
		sock_set_flag(sk, SOCK_DONE);
	...
		WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
	...
			if (sk->sk_state == TCP_SYN_SENT) {
				sk->sk_state = TCP_CLOSE;
				sk->sk_err = ECONNRESET;
				sk_error_report(sk);
				return;
			}
}

The valid qp_handle that this path requires is already installed while the
client socket is still TCP_SYN_SENT, by
vmci_transport_recv_connecting_client_negotiate(), which also subscribes
detach_sub_id; TCP_ESTABLISHED is only set later on ATTACH. The detach
callback runs under bh_lock_sock() while blocking connect() has dropped the
socket lock in release_sock()/schedule_timeout().

The woken connect() then sees sk_state == TCP_CLOSE, so the new
ESTABLISHED/TCP_CLOSING short circuit above does not apply, it leaves the
loop on sk_err != 0, takes err = sock_error(sk) and reaches
vsock_unconnected_reset(), where the first check fires and returns:

net/vmw_vsock/af_vsock.c:vsock_unconnected_reset() {
	if (WARN_ON_ONCE(vsk->peer_shutdown) ||
	    WARN_ON_ONCE(sock_flag(sk, SOCK_DONE)) ||
	    WARN_ON_ONCE(sk->sk_state != TCP_SYN_SENT &&
			 sk->sk_state != TCP_CLOSE))
		return;
	...
}

That skips vsock_transport_cancel_pkt(), vsock_deassign_transport(), the
WRITE_ONCE(sk->sk_state, TCP_CLOSE) and sk->sk_socket->state =
SS_UNCONNECTED.

Does that leave the socket with its transport still assigned and
sock->state stuck at SS_CONNECTING, so later connect() attempts return
-EALREADY for O_NONBLOCK, or re-enter the loop, burn the full
vsk->connect_timeout and return -ETIMEDOUT?

Before this series the same path simply did sk->sk_state = TCP_CLOSE;
sock->state = SS_UNCONNECTED; with no warning, and the socket stayed
retryable. At the end of the series the new SS_CONNECTED/SS_DISCONNECTING
early return in vsock_unconnected_reset() does not cover this case, since
sock->state is SS_CONNECTING here and sk_state is TCP_CLOSE, so the
WARN_ON_ONCE(vsk->peer_shutdown) check is still the one that fires.

>  out:
>  	release_sock(sk);
>  	return err;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-vsock-connect-reset-closing-v3-0-78907b8200d4%40rbox.co