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 <[email protected]>
> >> ---
> >> 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
