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

[Stefano Garzarella <sgarzare@redhat.com>]

On Tue, Sep 22, 2026 at 03:18:05PM +0200, Michal Luczaj wrote:
> 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@rbox.co>
> >> ---
> >> 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.

Sure, as you prefer, I'm fine to postpone this.

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

Honestly, it didn't seem right to me either; I think we're fine the way 
we are for now.

Thanks,
Stefano