Re: [PATCH net v2 5/5] vsock: Handle sudden TCP_CLOSE during connect

[Michal Luczaj]

On 9/16/26 14:31, Stefano Garzarella wrote:
> On Tue, Sep 15, 2026 at 03:15:16PM +0200, Michal Luczaj wrote:
>> Virtio/PM events are serviced by virtio_vsock_reset_sock(), which resets
> 
> What "PM" means here?

Power Management, namely virtio_vsock_driver.freeze which is defined under
CONFIG_PM_SLEEP. I'll generalize this comment, as suggested below.

>> each connected socket. The reset is done under vsock_table_lock but without
>> taking lock_sock(), so from the point of view of vsock_connect() -
>> locklessly. The same pattern exists in VMCI's
>> vmci_transport_handle_detach() and vhost's vhost_vsock_reset_orphans().
>>
>> The complexity of connect() comes from the fact that:
>> 1. the virtio transport can be reassigned, so the old transport must be
>>   safely released;
>> 2. a failed connect can be followed by a retry, so the socket must be
>>   reverted to a sensible state.
>> Both cases apply only as long as the socket has not yet established a
>> connection.
>>
>> While connect() waits for TCP_SYN_SENT -> TCP_ESTABLISHED, other
>> transitions can also occur:
>>
>>  TCP_SYN_SENT -> TCP_CLOSE on connection failure, timeout or signal
>>  TCP_SYN_SENT -> TCP_ESTABLISHED -> TCP_CLOSING on VIRTIO_VSOCK_OP_RST
>>  TCP_SYN_SENT -> TCP_ESTABLISHED -> [TCP_CLOSING ->] TCP_CLOSE on event
>>
>> This further complicates connect(). Rather than making every event handler
>> drop the socket from connected_table or adapting connect() to handle more
>> transitions (while missing proper locking), use vsk->peer_shutdown as a
>> poison flag. Whatever state an event leaves the socket in, the flag bricks
>> it and prevents suspicious transport reassignments or TCP_SYN_SENT
>> retransmissions.
>>
>> Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
>> Signed-off-by: Michal Luczaj <mhal@xxxxxxx>
>> ---
>> net/vmw_vsock/af_vsock.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index adf3f018347e..972952d04a81 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -1743,6 +1743,12 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>> 			goto out;
>> 		}
>>
>> +		/* Virtio/PM events are serviced locklessly. */
> 
> IMO we should be generic here (i.e. don't mention virtio or mention it 
> like one of the transport, but IIUC also VMCI does something similar) 
> and also we should explain better why we are doing this, like you did in 
> the commit description.

Sure, will do.

> Maybe we should document this behaviour also on top of this file.

OK, do you mind if I do that as a follow up? I worry top of the file needs
few more updates.

>> +		if (READ_ONCE(vsk->peer_shutdown)) {
>> +			err = -ECONNRESET;
> 
> Is ECONNRESET a valid connect() error to return?
> 
>> +			goto out;
>> +		}
>> +
> 
>  From LLM reviewing, can you check if it's valid? :
>    - M (net/vmw_vsock/af_vsock.c:1747): VMCI regression.  vmci_transport_handle_detach() sets
>      peer_shutdown = SHUTDOWN_MASK unconditionally and then special-cases TCP_SYN_SENT with the
>      comment "we treat the detach event like a reset" — i.e. a connect() retry is the expected
>      recovery. It is reachable for a non-connected socket via vmci_transport_peer_detach_cb() (which
>      uses trans->sk, not the connected table). Since vsock_assign_transport() only clears
>      peer_shutdown when the transport actually changes (af_vsock.c:671-689), the retry now hits the
>      new check and returns -ECONNRESET forever: the fd is permanently bricked where it previously
>      reconnected.

I'm not sure I follow the logic. SHUTDOWN_MASK gets set no matter what's
the state at vmci_transport_handle_detach(). That means even if vmci's
reconnect succeeds, all the recvmsg()/sendmsg() at af_vsock layer would
fail anyway. Am I missing something?

thanks,
Michal