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

[Michal Luczaj <mhal@rbox.co>]

On 9/24/26 15:15, netdev-bot+sashiko@kernel.org wrote:
>> 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?

Yes.

> 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?

Do we want to support connect() being raced by shutdown()?

> [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?

Addressed in comments to patch #3.