Re: [PATCH net v3 3/8] vsock: do not preserve transport assignment

[Michal Luczaj <mhal@rbox.co>]

On 9/24/26 15:15, netdev-bot+sashiko@kernel.org wrote:
>> +static void vsock_unconnected_reset(struct sock *sk)
>> +{
>> +	struct vsock_sock *vsk = vsock_sk(sk);
>> +
>> +	sock_owned_by_me(sk);
>> +
>> +	/*
>> +	 * Only connected socks may have peer_shutdown or SOCK_DONE set.
>> +	 *
>> +	 * Once established (TCP_ESTABLISHED, TCP_CLOSING), a socket can be
>> +	 * de-assigned only on close(). But we can narrow the check down to
>> +	 * states we actually expect (TCP_SYN_SENT, TCP_CLOSE).
>> +	 */
>> +	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;
> 
> [Severity: High]
> Is the "only connected socks may have peer_shutdown or SOCK_DONE set"
> invariant true for VMCI?
> 
> vmci_transport_recv_connecting_client_negotiate() allocates the queue pair
> and subscribes to VMCI_EVENT_QP_PEER_DETACH while the socket is still in
> TCP_SYN_SENT. If the peer then detaches, vmci_transport_handle_detach()
> runs on a never-established socket:
> 
> The blocking connect() waiter then wakes on sk_err, err = sock_error(sk)
> becomes -ECONNRESET, and out_reset calls this helper with peer_shutdown ==
> SHUTDOWN_MASK and SOCK_DONE set.
> 
> Since WARN_ON_ONCE() evaluates to the condition, the helper splats and
> returns before vsock_transport_cancel_pkt(), vsock_deassign_transport() and
> the TCP_CLOSE/SS_UNCONNECTED restore. Can that leave the socket stuck with
> sk_socket->state == SS_CONNECTING and the transport (plus its module
> reference) pinned, so a later connect() returns -EALREADY for O_NONBLOCK or
> re-hits the same early return, and listen() returns -EINVAL?
> 
> Before this patch the same path unconditionally ran:
> 
> 	err = sock_error(sk);
> 	if (err) {
> 		sk->sk_state = TCP_CLOSE;
> 		sock->state = SS_UNCONNECTED;
> 	}
> 
> so the socket stayed retryable. Is the change in behaviour here intended?

True, I'll drop WARN_ON_ONCE(vsk->peer_shutdown).

> On panic_on_warn=1 kernels this is also reachable from an unprivileged
> connect() when the peer VM goes away mid-handshake.

It was meant for debugging and to trip the fuzzers. Would it be better to
use DEBUG_NET_WARN_ON_ONCE?

> [Severity: High]
> Can this de-assign the transport of a socket that did reach
> TCP_ESTABLISHED?
> ...
> A later patch in this series, "vsock: handle socket bricking due to
> transport event", adds an early return on sk_socket->state == SS_CONNECTED
> or SS_DISCONNECTING at the top of this helper, which covers exactly this
> case, so this only affects this commit taken on its own. Would it be
> cleaner to fold that guard in here?

Maybe it would be cleaner, but I think it's easier to review as it is now.

> [Severity: Medium]
> For a non-blocking connect() whose failure is detected asynchronously by
> the transport, which path reaches out_reset?
> 
> connect(fd, O_NONBLOCK) assigns a transport, sets sk_state = TCP_SYN_SENT,
> sock->state = SS_CONNECTING and returns -EINPROGRESS. If the peer answers
> with VIRTIO_VSOCK_OP_RST, virtio_transport_recv_connecting() takes its
> destroy label:
> 
> net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_connecting() {
> 	...
> destroy:
> 	virtio_transport_reset(vsk, skb);
> 	sk->sk_state = TCP_CLOSE;
> 	sk->sk_err = skerr;
> 	sk_error_report(sk);
> 	...
> }
> 
> sk_socket->state is left at SS_CONNECTING and the transport stays assigned.
> vmci_transport_recv_connecting_client() has the same pattern.
> 
> vsock_connect_timeout() above is gated on sk->sk_state == TCP_SYN_SENT, so
> it does not reset either once the state is TCP_CLOSE. And a retry with
> O_NONBLOCK hits:
> 
> 	case SS_CONNECTING:
> 		...
> 		err = -EALREADY;
> 		if (flags & O_NONBLOCK)
> 			goto out;
> 
> which returns before out_reset.
> 
> Does the socket then keep the transport assignment and the transport module
> reference until close()? The changelog says:
> 
>     If connection fails (init went wrong, peer misbehaviour, time out,
>     signal), transport is de-assigned and socket state is re-initialized.
> 
> Should the asynchronous peer-misbehaviour case for a non-blocking connect()
> be covered as well, or should the changelog be narrowed to the synchronous
> paths?
OK, so a _non-blocking_ connect() getting hit with RST before connection
can be established: fd bricked, you can't re-connect() with it. While you
can retry a _blocking_ connect getting hit the same way. And the choice is
to unify the behaviour or document the discrepancy, if I get it right.