On 9/24/26 15:15, [email protected] 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.
