Re: [PATCH net v3 6/8] vsock: handle socket bricking due to transport event

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Over-broad new guard in vsock_unconnected_reset(): the early
  return also fires for sockets that were never established.
- [High] Incomplete race guard: vmci_transport_handle_detach() can brick
  a socket that is still TCP_SYN_SENT / SS_CONNECTING, which the new…

--

vsock: handle socket bricking due to transport event

Transport events reset connected sockets to TCP_CLOSE without holding
lock_sock(), so connect() can observe the reset concurrently.  This patch
makes vsock_unconnected_reset() bail out early when the socket-level state
is SS_CONNECTED or SS_DISCONNECTING, on the premise that transport events
leave SS_CONNECTED unchanged.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index ff4140aaf1f32..5a2d7e10ecb88 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1663,6 +1663,19 @@ static void vsock_unconnected_reset(struct sock *sk)
>  
>  	sock_owned_by_me(sk);
>  
> +	/*
> +	 * Transport events are serviced by resetting each connected socket to
> +	 * TCP_CLOSE. The reset is done under vsock_table_lock but without
> +	 * taking lock_sock(), so it is effectively lockless from the
> +	 * perspective of connect().
> +	 *
> +	 * Preserve socket's bricked status. Also check SS_DISCONNECTING, since
> +	 * shutdown() may race us.
> +	 */
> +	if (sk->sk_socket->state == SS_CONNECTED ||
> +	    sk->sk_socket->state == SS_DISCONNECTING)
> +		return;
> +

[Severity: High]
Can the SS_DISCONNECTING part of this test also swallow the reset for a
socket that never established?

vsock_shutdown() moves any socket that is not SS_UNCONNECTED to
SS_DISCONNECTING, including one that is merely SS_CONNECTING /
TCP_SYN_SENT:

net/vmw_vsock/af_vsock.c:vsock_shutdown() {
	...
	if (sock->state == SS_UNCONNECTED) {
		err = -ENOTCONN;
		if (sock_type_connectible(sk->sk_type))
			goto out;
	} else {
		sock->state = SS_DISCONNECTING;
		err = 0;
	}
	...
}

For a blocking connect(), thread A sleeps in schedule_timeout() with the
socket lock released, thread B calls shutdown(fd, SHUT_RD).  sk_state stays
TCP_SYN_SENT and sk_err stays 0, so the wait loop keeps sleeping until the
timeout or a signal, then takes out_wait -> out_reset ->
vsock_unconnected_reset(), which now returns here because sock->state is
SS_DISCONNECTING.

For a non-blocking connect(), the same guard in vsock_connect_timeout()
does not filter a SHUT_RD-only shutdown:

net/vmw_vsock/af_vsock.c:vsock_connect_timeout() {
	...
	if (sk->sk_state == TCP_SYN_SENT &&
	    (sk->sk_shutdown != SHUTDOWN_MASK)) {
		sk->sk_err = ETIMEDOUT;
		sk_error_report(sk);
		vsock_unconnected_reset(sk);
	}
	...
}

so the worker calls the helper and the reset is swallowed there too.

In both cases vsock_transport_cancel_pkt(), vsock_deassign_transport() and
the WRITE_ONCE(sk->sk_state, TCP_CLOSE) / sk->sk_socket->state =
SS_UNCONNECTED below are skipped, and no further timeout is armed.  Does
that leave the socket stuck in TCP_SYN_SENT with the transport still
assigned and the VIRTIO_VSOCK_OP_REQUEST not cancelled?

A late response then still matches TCP_SYN_SENT:

net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_connecting() {
	...
	case VIRTIO_VSOCK_OP_RESPONSE:
		sk->sk_state = TCP_ESTABLISHED;
		sk->sk_socket->state = SS_CONNECTED;
		vsock_insert_connected(vsk);
	...
}

so the connection can complete and become usable for send()/recv() after
connect() already returned -ETIMEDOUT or -EINTR.  A retry of connect() now
hits case SS_DISCONNECTING: err = -EINVAL, whereas before this patch the
failure path reset the socket to TCP_CLOSE / SS_UNCONNECTED.

Would an explicit flag, or a vsock_connected_table membership test, express
the intended "socket was once established" condition better than
sock->state?

[Severity: High]
The commit message says "But transport events leave SS_CONNECTED state
unchanged" and lists vmci_transport_handle_detach() as one of the handlers.
Does that hold for the VMCI detach path?

vmci_transport_handle_detach() runs from vmci_transport_peer_detach_cb()
under trans->lock plus bh_lock_sock() only, i.e. concurrently with a
connect() that released the socket lock in schedule_timeout(), and it
mutates a socket that is still in the handshake:

net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
	if (!vmci_handle_is_invalid(vmci_trans(vsk)->qp_handle)) {
		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;
	...
}

A client socket already owns a valid qp_handle while still TCP_SYN_SENT,
because vmci_transport_recv_connecting_client_negotiate() publishes
vmci_trans(vsk)->qp_handle and detach_sub_id before ATTACH arrives;
SS_CONNECTED is only set later in the ATTACH branch of
vmci_transport_recv_connecting_client().

Since the handler never touches sk->sk_socket->state, connect() left it at
SS_CONNECTING, so the new check does not match.  connect()'s loop exits on
sk_err != 0 and reaches out_reset -> vsock_unconnected_reset(), where the
existing checks below fire:

	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;

Can a peer VM detaching mid-handshake therefore still produce these
WARN_ON_ONCE() splats and still skip vsock_transport_cancel_pkt() and
vsock_deassign_transport(), leaving the stale transport assignment the
patch aims to avoid?

This appears to be still present at the end of the series; the later commit
6b7829ad only adds the TCP_LISTEN check.

>  	/*
>  	 * Only connected socks may have peer_shutdown or SOCK_DONE set.
>  	 *

Thanks for looking into these.

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