[PATCH net v3 0/8] vsock: fix connect() races

[PATCH net v3 0/8] vsock: fix connect() races

[Michal Luczaj]

Handle racy socket changes in connect().

v3 is taking a different approach by turning transport assignment on its
head: instead of trying to preserve the transport between failed
connect()s, drop it from an _unconnected_ socket as soon as possible, i.e.
when something goes wrong with the init or binding, connection fails, time
outs or is interrupted by a signal. IOW, route the failure paths of
vsock_connect() and vsock_connect_timeout() through one point, replacing
the open coded state rollback. Starting from there, corner cases are
handled, invariant enforced.

What's perhaps more important, the bug fixes themselves do not touch the
driver code and do not introduce lockless writes.

---
Changes in v3:
- Change the transport assignment life cycle (Stafano)
- Expand comments (Stafano)
- CC: drop bouncing address
- Link to v2: https://patch.msgid.link/20260915-vsock-connect-reset-closing-v2-0-a1d9abb472f7@rbox.co

Changes in v2:
- CC: drop bouncing addresses
- Add more related fixes
- Handle ENOBUFS (Bobby)
- Consume sk_err (Stefano)
- Link to v1: https://patch.msgid.link/20260909-vsock-connect-reset-closing-v1-1-50298b9ccfbf@rbox.co

---
Michal Luczaj (8):
      vhost/vsock: Fix socket state constant
      vsock: simplify error check condition
      vsock: do not preserve transport assignment
      vsock: drop the re-assignment logic
      vsock: treat TCP_CLOSING as once-established
      vsock: handle socket bricking due to transport event
      vsock: handle listen() racing connect()
      vsock: enforce no-transport invariant for TCP_LISTEN sockets

 drivers/vhost/vsock.c    |   2 +-
 net/vmw_vsock/af_vsock.c | 142 +++++++++++++++++++++++++++--------------------
 2 files changed, 82 insertions(+), 62 deletions(-)
---
base-commit: 23d42b9a3bcd55b17d3b371544fedc708c2397e9
change-id: 20260820-vsock-connect-reset-closing-98dd28769d7f

Best regards,
--  
Michal Luczaj <mhal@rbox.co>

[PATCH net v3 1/8] vhost/vsock: Fix socket state constant

[Michal Luczaj]

Refactor under Fixes missed one: SS_UNCONNECTED (1) -> TCP_CLOSE (7).
Until now, sk->sk_state = 1 stood for TCP_ESTABLISHED.

Fixes: 3b4477d2dcf2 ("VSOCK: use TCP state constants for sk_state")
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 drivers/vhost/vsock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
index abed1fbcf66c..6051533456f6 100644
--- a/drivers/vhost/vsock.c
+++ b/drivers/vhost/vsock.c
@@ -774,7 +774,7 @@ static void vhost_vsock_reset_orphans(struct sock *sk)
 
 	sock_set_flag(sk, SOCK_DONE);
 	vsk->peer_shutdown = SHUTDOWN_MASK;
-	sk->sk_state = SS_UNCONNECTED;
+	sk->sk_state = TCP_CLOSE;
 	sk->sk_err = ECONNRESET;
 	sk_error_report(sk);
 }

-- 
2.55.0

[PATCH net v3 2/8] vsock: simplify error check condition

[Michal Luczaj]

On success, vsock_assign_transport() ensures transport != NULL.
Drop the redundant check.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 9b71479a2b29..5c8e7e7d35b4 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1757,8 +1757,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 		/* The hypervisor and well-known contexts do not have socket
 		 * endpoints.
 		 */
-		if (!transport ||
-		    !transport->stream_allow(vsk, remote_addr->svm_cid,
+		if (!transport->stream_allow(vsk, remote_addr->svm_cid,
 					     remote_addr->svm_port)) {
 			err = -ENETUNREACH;
 			goto out;

-- 
2.55.0

[PATCH net v3 3/8] vsock: do not preserve transport assignment

[Michal Luczaj]

Introduce vsock_unconnected_reset() and adapt vsock_connect().

Change transport assignment life cycle. On connect(), socket gets a
transport assigned. If connection fails (init went wrong, peer
misbehaviour, time out, signal), transport is de-assigned and socket state
is re-initialized. Once the connection is established, transport remains
assigned until close().

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 74 ++++++++++++++++++++++++++++++------------------
 1 file changed, 47 insertions(+), 27 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 5c8e7e7d35b4..20181ddde114 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1679,6 +1679,42 @@ static int vsock_transport_cancel_pkt(struct vsock_sock *vsk)
 	return transport->cancel_pkt(vsk);
 }
 
+static void vsock_unconnected_reset(struct sock *sk)
+{
+	struct vsock_sock *vsk = vsock_sk(sk);
+
+	sock_owned_by_me(sk);
+
+	/*
+	 * Only connected socks may have peer_shutdown or SOCK_DONE set.
+	 *
+	 * Once established (TCP_ESTABLISHED, TCP_CLOSING), a socket can be
+	 * de-assigned only on close(). But we can narrow the check down to
+	 * states we actually expect (TCP_SYN_SENT, TCP_CLOSE).
+	 */
+	if (WARN_ON_ONCE(vsk->peer_shutdown) ||
+	    WARN_ON_ONCE(sock_flag(sk, SOCK_DONE)) ||
+	    WARN_ON_ONCE(sk->sk_state != TCP_SYN_SENT &&
+			 sk->sk_state != TCP_CLOSE))
+		return;
+
+	/*
+	 * Try to cancel a VIRTIO_VSOCK_OP_REQUEST skb that may have been sent
+	 * out by transport->connect().
+	 */
+	vsock_transport_cancel_pkt(vsk);
+
+	/*
+	 * No need to invoke transport->release() for unconnected connectible
+	 * sockets. Go straight for transport deassign.
+	 */
+	vsock_deassign_transport(vsk);
+
+	/* Revert socket to initial state. Keep sk_err. */
+	WRITE_ONCE(sk->sk_state, TCP_CLOSE);
+	sk->sk_socket->state = SS_UNCONNECTED;
+}
+
 static void vsock_connect_timeout(struct work_struct *work)
 {
 	struct sock *sk;
@@ -1690,11 +1726,9 @@ static void vsock_connect_timeout(struct work_struct *work)
 	lock_sock(sk);
 	if (sk->sk_state == TCP_SYN_SENT &&
 	    (sk->sk_shutdown != SHUTDOWN_MASK)) {
-		sk->sk_state = TCP_CLOSE;
-		sk->sk_socket->state = SS_UNCONNECTED;
 		sk->sk_err = ETIMEDOUT;
 		sk_error_report(sk);
-		vsock_transport_cancel_pkt(vsk);
+		vsock_unconnected_reset(sk);
 	}
 	release_sock(sk);
 
@@ -1760,7 +1794,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 		if (!transport->stream_allow(vsk, remote_addr->svm_cid,
 					     remote_addr->svm_port)) {
 			err = -ENETUNREACH;
-			goto out;
+			goto out_reset;
 		}
 
 		if (vsock_msgzerocopy_allow(transport)) {
@@ -1771,18 +1805,18 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 			 * feature is supported here.
 			 */
 			err = -EOPNOTSUPP;
-			goto out;
+			goto out_reset;
 		}
 
 		err = vsock_auto_bind(vsk);
 		if (err)
-			goto out;
+			goto out_reset;
 
 		sk->sk_state = TCP_SYN_SENT;
 
 		err = transport->connect(vsk);
 		if (err < 0)
-			goto out;
+			goto out_reset;
 
 		/* sk_err might have been set as a result of an earlier
 		 * (failed) connect attempt.
@@ -1825,8 +1859,9 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 					     timeout))
 				sock_put(sk);
 
+			finish_wait(sk_sleep(sk), &wait);
 			/* Skip ahead to preserve error code set above. */
-			goto out_wait;
+			goto out;
 		}
 
 		release_sock(sk);
@@ -1844,12 +1879,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 			break;
 
 		/* If connection was _not_ established and a signal/timeout came
-		 * to be, we want the socket's state reset. User space may want
-		 * to retry.
-		 *
-		 * sk_state != TCP_ESTABLISHED implies that socket is not on
-		 * vsock_connected_table. We keep the binding and the transport
-		 * assigned.
+		 * to be, we want the socket's state reset. We keep the binding.
 		 */
 		if (signal_pending(current) || timeout == 0) {
 			err = timeout == 0 ? -ETIMEDOUT : sock_intr_errno(timeout);
@@ -1859,14 +1889,6 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 			 * sk_state == TCP_SYN_SENT, which hereby we break.
 			 * In such case VIRTIO_VSOCK_OP_RST will follow.
 			 */
-			sk->sk_state = TCP_CLOSE;
-			sock->state = SS_UNCONNECTED;
-
-			/* Try to cancel VIRTIO_VSOCK_OP_REQUEST skb sent out by
-			 * transport->connect().
-			 */
-			vsock_transport_cancel_pkt(vsk);
-
 			goto out_wait;
 		}
 
@@ -1874,13 +1896,11 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 	}
 
 	err = sock_error(sk);
-	if (err) {
-		sk->sk_state = TCP_CLOSE;
-		sock->state = SS_UNCONNECTED;
-	}
-
 out_wait:
 	finish_wait(sk_sleep(sk), &wait);
+out_reset:
+	if (err)
+		vsock_unconnected_reset(sk);
 out:
 	release_sock(sk);
 	return err;

-- 
2.55.0

[PATCH net v3 4/8] vsock: drop the re-assignment logic

[Michal Luczaj]

On a failed connect(), transport is de-assigned. Connected socket keeps
transport until close(). Hence, no more re-assigning.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 20181ddde114..5cecd1c9c6e5 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -604,6 +604,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 	__u8 remote_flags;
 	int ret;
 
+	if (WARN_ON_ONCE(vsk->transport || vsk->trans))
+		return -EINVAL;
+
 	/* If the packet is coming with the source and destination CIDs higher
 	 * than VMADDR_CID_HOST, then a vsock channel where all the packets are
 	 * forwarded to the host should be established. Then the host will
@@ -649,11 +652,6 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 		goto err;
 	}
 
-	if (vsk->transport && vsk->transport == new_transport) {
-		ret = 0;
-		goto err;
-	}
-
 	/* We increase the module refcnt to prevent the transport unloading
 	 * while there are open sockets assigned to it.
 	 */
@@ -668,26 +666,6 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
 	 */
 	mutex_unlock(&vsock_register_mutex);
 
-	if (vsk->transport) {
-		/* transport->release() must be called with sock lock acquired.
-		 * This path can only be taken during vsock_connect(), where we
-		 * have already held the sock lock. In the other cases, this
-		 * function is called on a new socket which is not assigned to
-		 * any transport.
-		 */
-		vsk->transport->release(vsk);
-		vsock_deassign_transport(vsk);
-
-		/* transport's release() and destruct() can touch some socket
-		 * state, since we are reassigning the socket to a new transport
-		 * during vsock_connect(), let's reset these fields to have a
-		 * clean state.
-		 */
-		sock_reset_flag(sk, SOCK_DONE);
-		sk->sk_state = TCP_CLOSE;
-		WRITE_ONCE(vsk->peer_shutdown, 0);
-	}
-
 	if (sk->sk_type == SOCK_SEQPACKET) {
 		if (!new_transport->seqpacket_allow ||
 		    !new_transport->seqpacket_allow(vsk, remote_cid)) {

-- 
2.55.0

[PATCH net v3 5/8] vsock: treat TCP_CLOSING as once-established

[Michal Luczaj]

When connect() is scheduled out awaiting TCP_SYN_SENT -> TCP_ESTABLISHED,
socket may already transition TCP_ESTABLISHED -> TCP_CLOSING
(VIRTIO_VSOCK_OP_RST) or have sk_err set (VIRTIO_VSOCK_OP_RW). Short
circuit the logic; do not assume a non-zero sk_err signifies a failed
connect. connect() should return the status of connection attempt itself,
not what followed.

Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Link: https://lore.kernel.org/netdev/anzT1fREOSyHT99k@v4bel/
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 5cecd1c9c6e5..ff4140aaf1f3 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1846,15 +1846,19 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 		timeout = schedule_timeout(timeout);
 		lock_sock(sk);
 
-		/* Connection established. Whatever happens to socket once we
-		 * release it, that's not connect()'s concern. No need to go
+		/* Connection was established. Whatever happens to socket once
+		 * we release it, that's not connect()'s concern. No need to go
 		 * into signal and timeout handling. Call it a day.
 		 *
 		 * Note that allowing to "reset" an already established socket
 		 * here is racy and insecure.
 		 */
-		if (sk->sk_state == TCP_ESTABLISHED)
-			break;
+		if (sk->sk_state == TCP_ESTABLISHED ||
+		    sk->sk_state == TCP_CLOSING) {
+			err = 0;
+			finish_wait(sk_sleep(sk), &wait);
+			goto out;
+		}
 
 		/* If connection was _not_ established and a signal/timeout came
 		 * to be, we want the socket's state reset. We keep the binding.
@@ -1877,8 +1881,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
 out_wait:
 	finish_wait(sk_sleep(sk), &wait);
 out_reset:
-	if (err)
-		vsock_unconnected_reset(sk);
+	vsock_unconnected_reset(sk);
 out:
 	release_sock(sk);
 	return err;

-- 
2.55.0

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

[Michal Luczaj]

Transport events are serviced by resetting 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 is present
in virtio_vsock_reset_sock(), vmci_transport_handle_detach() and
vhost_vsock_reset_orphans().

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

But transport events leave SS_CONNECTED state unchanged. So take a note of
that, rather than a) making every event handler drop the socket from
connected_table, or b) adapting connect() to handle more transitions (while
missing proper locking).

Fixes: d021c344051a ("VSOCK: Introduce VM Sockets")
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index ff4140aaf1f3..5a2d7e10ecb8 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;
+
 	/*
 	 * Only connected socks may have peer_shutdown or SOCK_DONE set.
 	 *

-- 
2.55.0

[PATCH net v3 7/8] vsock: handle listen() racing connect()

[Michal Luczaj]

Socket can flip to TCP_LISTEN when connect() schedules out. Respect
listen() winning the race: let connect() time out, but don't reset the
socket.

sk is CLOSE UNCONNECTED
non-blocking connect():
  sk := SYN_SENT CONNECTING
  enqueue vsock_connect_timeout()
                                   blocking connect():
                                     release_sock()
                                     schedule_timeout()
vsock_connect_timeout():
  sk := CLOSE UNCONNECTED
listen():
  sk := LISTEN UNCONNECTED
                                     lock_sock()
                                     sk is TCP_LISTEN UNCONNECTED

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 5a2d7e10ecb8..56c595942456 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1663,6 +1663,13 @@ static void vsock_unconnected_reset(struct sock *sk)
 
 	sock_owned_by_me(sk);
 
+	/*
+	 * listen() can race connect() on schedule_timeout().
+	 * Don't reset TCP_LISTEN.
+	 */
+	if (sk->sk_state == TCP_LISTEN)
+		return;
+
 	/*
 	 * Transport events are serviced by resetting each connected socket to
 	 * TCP_CLOSE. The reset is done under vsock_table_lock but without

-- 
2.55.0

[PATCH net v3 8/8] vsock: enforce no-transport invariant for TCP_LISTEN sockets

[Michal Luczaj]

In the past, listener socket could carry an assigned transport (uselessly,
but harmlessly, except for unnecessarily hogging the kernel module refcnt).
That's not the case anymore. Express/enforce the invariant.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 net/vmw_vsock/af_vsock.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 56c595942456..9ba21a448cfa 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1993,13 +1993,13 @@ static int vsock_listen(struct socket *sock, int backlog)
 		goto out;
 	}
 
-	if (sock->state != SS_UNCONNECTED) {
+	vsk = vsock_sk(sk);
+
+	if (sock->state != SS_UNCONNECTED || WARN_ON_ONCE(vsk->transport)) {
 		err = -EINVAL;
 		goto out;
 	}
 
-	vsk = vsock_sk(sk);
-
 	if (!vsock_addr_bound(&vsk->local_addr)) {
 		err = -EINVAL;
 		goto out;

-- 
2.55.0

Re: [PATCH net v3 3/8] vsock: do not preserve transport assignment

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The WARN_ON_ONCE() assertions added in vsock_unconnected_reset()
  (af_vsock.c:1695-1698) are reachable from ordinary peer behaviour, and…
- [High] vsock_unconnected_reset() infers 'never established' from
  sk_state/peer_shutdown/SOCK_DONE only.
- [Medium] Incomplete lifecycle change vs. commit message: for a non-
  blocking connect() the new de-assign path is never reached when the…

--

vsock: do not preserve transport assignment

This adds vsock_unconnected_reset() and reworks vsock_connect() so that a
failed connection attempt de-assigns the transport and reverts the socket
to its initial state, while an established socket keeps its transport until
close().

A few questions on the new helper and on the paths that reach it.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 5c8e7e7d35b4f..20181ddde114f 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1679,6 +1679,42 @@ static int vsock_transport_cancel_pkt(struct vsock_sock *vsk)
>  	return transport->cancel_pkt(vsk);
>  }
>  
> +static void vsock_unconnected_reset(struct sock *sk)
> +{
> +	struct vsock_sock *vsk = vsock_sk(sk);
> +
> +	sock_owned_by_me(sk);
> +
> +	/*
> +	 * Only connected socks may have peer_shutdown or SOCK_DONE set.
> +	 *
> +	 * Once established (TCP_ESTABLISHED, TCP_CLOSING), a socket can be
> +	 * de-assigned only on close(). But we can narrow the check down to
> +	 * states we actually expect (TCP_SYN_SENT, TCP_CLOSE).
> +	 */
> +	if (WARN_ON_ONCE(vsk->peer_shutdown) ||
> +	    WARN_ON_ONCE(sock_flag(sk, SOCK_DONE)) ||
> +	    WARN_ON_ONCE(sk->sk_state != TCP_SYN_SENT &&
> +			 sk->sk_state != TCP_CLOSE))
> +		return;

[Severity: High]
Is the "only connected socks may have peer_shutdown or SOCK_DONE set"
invariant true for VMCI?

vmci_transport_recv_connecting_client_negotiate() allocates the queue pair
and subscribes to VMCI_EVENT_QP_PEER_DETACH while the socket is still in
TCP_SYN_SENT. If the peer then detaches, vmci_transport_handle_detach()
runs on a never-established socket:

net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
	...
		sock_set_flag(sk, SOCK_DONE);
	...
		WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
	...
			if (sk->sk_state == TCP_SYN_SENT) {
	...
				sk->sk_state = TCP_CLOSE;
				sk->sk_err = ECONNRESET;
				sk_error_report(sk);
	...
}

The blocking connect() waiter then wakes on sk_err, err = sock_error(sk)
becomes -ECONNRESET, and out_reset calls this helper with peer_shutdown ==
SHUTDOWN_MASK and SOCK_DONE set.

Since WARN_ON_ONCE() evaluates to the condition, the helper splats and
returns before vsock_transport_cancel_pkt(), vsock_deassign_transport() and
the TCP_CLOSE/SS_UNCONNECTED restore. Can that leave the socket stuck with
sk_socket->state == SS_CONNECTING and the transport (plus its module
reference) pinned, so a later connect() returns -EALREADY for O_NONBLOCK or
re-hits the same early return, and listen() returns -EINVAL?

Before this patch the same path unconditionally ran:

	err = sock_error(sk);
	if (err) {
		sk->sk_state = TCP_CLOSE;
		sock->state = SS_UNCONNECTED;
	}

so the socket stayed retryable. Is the change in behaviour here intended?

On panic_on_warn=1 kernels this is also reachable from an unprivileged
connect() when the peer VM goes away mid-handshake.

> +
> +	/*
> +	 * Try to cancel a VIRTIO_VSOCK_OP_REQUEST skb that may have been sent
> +	 * out by transport->connect().
> +	 */
> +	vsock_transport_cancel_pkt(vsk);
> +
> +	/*
> +	 * No need to invoke transport->release() for unconnected connectible
> +	 * sockets. Go straight for transport deassign.
> +	 */
> +	vsock_deassign_transport(vsk);

[Severity: High]
Can this de-assign the transport of a socket that did reach
TCP_ESTABLISHED?

virtio_vsock_reset_sock() (and vhost_vsock_reset_orphans()) move an
established socket to TCP_CLOSE without lock_sock(), holding only
vsock_table_lock:

net/vmw_vsock/virtio_transport.c:virtio_vsock_reset_sock() {
	...
	sk->sk_state = TCP_CLOSE;
	sk->sk_err = ECONNRESET;
	sk_error_report(sk);
	...
}

sk_socket->state stays SS_CONNECTED and the socket stays linked in
vsock_connected_table. A blocking connect() woken after the handshake
completed then reaches out_reset with err = -ECONNRESET, and all three
guards above pass because they only look at sk_state, peer_shutdown and
SOCK_DONE.

virtio_transport_destruct() frees the transport state without purging
vvs->rx_queue and without unhashing the socket:

net/vmw_vsock/virtio_transport_common.c:virtio_transport_destruct() {
	virtio_transport_cancel_close_work(vsk, true);

	kfree(vvs);
	vsk->trans = NULL;
	...
}

The rx_queue purge lives in virtio_transport_remove_sock(), which is only
reached via release(), deliberately skipped here. Does this leak the queued
rx skbs, and does a subsequent successful connect() perform a second
list_add() on the still-linked connected_table node in
__vsock_insert_connected()?

A later patch in this series, "vsock: handle socket bricking due to
transport event", adds an early return on sk_socket->state == SS_CONNECTED
or SS_DISCONNECTING at the top of this helper, which covers exactly this
case, so this only affects this commit taken on its own. Would it be
cleaner to fold that guard in here?

> +
> +	/* Revert socket to initial state. Keep sk_err. */
> +	WRITE_ONCE(sk->sk_state, TCP_CLOSE);
> +	sk->sk_socket->state = SS_UNCONNECTED;
> +}
> +
>  static void vsock_connect_timeout(struct work_struct *work)
>  {
>  	struct sock *sk;
> @@ -1690,11 +1726,9 @@ static void vsock_connect_timeout(struct work_struct *work)
>  	lock_sock(sk);
>  	if (sk->sk_state == TCP_SYN_SENT &&
>  	    (sk->sk_shutdown != SHUTDOWN_MASK)) {
> -		sk->sk_state = TCP_CLOSE;
> -		sk->sk_socket->state = SS_UNCONNECTED;
>  		sk->sk_err = ETIMEDOUT;
>  		sk_error_report(sk);
> -		vsock_transport_cancel_pkt(vsk);
> +		vsock_unconnected_reset(sk);
>  	}
>  	release_sock(sk);
>  

[ ... ]

> @@ -1874,13 +1896,11 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  	}
>  
>  	err = sock_error(sk);
> -	if (err) {
> -		sk->sk_state = TCP_CLOSE;
> -		sock->state = SS_UNCONNECTED;
> -	}
> -
>  out_wait:
>  	finish_wait(sk_sleep(sk), &wait);
> +out_reset:
> +	if (err)
> +		vsock_unconnected_reset(sk);
>  out:
>  	release_sock(sk);
>  	return err;

[Severity: Medium]
For a non-blocking connect() whose failure is detected asynchronously by
the transport, which path reaches out_reset?

connect(fd, O_NONBLOCK) assigns a transport, sets sk_state = TCP_SYN_SENT,
sock->state = SS_CONNECTING and returns -EINPROGRESS. If the peer answers
with VIRTIO_VSOCK_OP_RST, virtio_transport_recv_connecting() takes its
destroy label:

net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_connecting() {
	...
destroy:
	virtio_transport_reset(vsk, skb);
	sk->sk_state = TCP_CLOSE;
	sk->sk_err = skerr;
	sk_error_report(sk);
	...
}

sk_socket->state is left at SS_CONNECTING and the transport stays assigned.
vmci_transport_recv_connecting_client() has the same pattern.

vsock_connect_timeout() above is gated on sk->sk_state == TCP_SYN_SENT, so
it does not reset either once the state is TCP_CLOSE. And a retry with
O_NONBLOCK hits:

	case SS_CONNECTING:
		...
		err = -EALREADY;
		if (flags & O_NONBLOCK)
			goto out;

which returns before out_reset.

Does the socket then keep the transport assignment and the transport module
reference until close()? The changelog says:

    If connection fails (init went wrong, peer misbehaviour, time out,
    signal), transport is de-assigned and socket state is re-initialized.

Should the asynchronous peer-misbehaviour case for a non-blocking connect()
be covered as well, or should the changelog be narrowed to the synchronous
paths?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-vsock-connect-reset-closing-v3-0-78907b8200d4%40rbox.co

Re: [PATCH net v3 4/8] vsock: drop the re-assignment logic

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The invariant this patch asserts ("On a failed connect(),
  transport is de-assigned", enforced by the new…
- [Low] Two in-tree comments still justify the SOCK_DEAD gating in
  vsock_remove_sock() by the "transport reassignment" case, which…
- [Low] vsock_assign_transport() gains a hard precondition (the socket
  must have neither ->transport nor ->trans set, otherwise WARN +…

--

vsock: drop the re-assignment logic

This removes the transport re-assignment path from vsock_assign_transport()
and replaces it with an assertion that the socket has no transport and no
private transport state yet.  The rationale given is that a failed
connect() de-assigns the transport and a connected socket keeps its
transport until close().

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 20181ddde114f..5cecd1c9c6e50 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -604,6 +604,9 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>  	__u8 remote_flags;
>  	int ret;
>  
> +	if (WARN_ON_ONCE(vsk->transport || vsk->trans))
> +		return -EINVAL;
> +

[Severity: Medium]
Is the invariant behind this assertion always true?  The commit message
says "On a failed connect(), transport is de-assigned", but
vsock_unconnected_reset() can return early without de-assigning:

net/vmw_vsock/af_vsock.c:vsock_unconnected_reset() {
	if (WARN_ON_ONCE(vsk->peer_shutdown) ||
	    WARN_ON_ONCE(sock_flag(sk, SOCK_DONE)) ||
	    WARN_ON_ONCE(sk->sk_state != TCP_SYN_SENT &&
			 sk->sk_state != TCP_CLOSE))
		return;
	...
	vsock_deassign_transport(vsk);
	...
}

With the VMCI transport, can a peer detach during the handshake produce
exactly that state?  vmci_transport_recv_connecting_client_negotiate()
subscribes the detach callback while sk_state is still TCP_SYN_SENT, and
then:

net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
	sock_set_flag(sk, SOCK_DONE);

	/* On a detach the peer will not be sending or receiving
	 * anymore.
	 */
	WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
	...
		if (sk->sk_state == TCP_SYN_SENT) {
	...
			sk->sk_state = TCP_CLOSE;
			sk->sk_err = ECONNRESET;
}

The blocking connect() then wakes on sk_err and takes:

net/vmw_vsock/af_vsock.c:vsock_connect() {
out_reset:
	if (err)
		vsock_unconnected_reset(sk);
}

so WARN_ON_ONCE(vsk->peer_shutdown) fires and the transport is never
de-assigned, leaving sock->state at SS_CONNECTING until close().  A later
vsock_assign_transport() on that socket would then hit the new assertion
added here.

At this commit there also seems to be a second, transport-independent way
in: a peer RESPONSE followed by RST leaves the socket in TCP_CLOSING with
SOCK_DONE and peer_shutdown set while connect() sleeps, and since the wait
loop here only breaks on TCP_ESTABLISHED, a signal or the connect timeout
routes to out_reset with err != 0 and trips the same warnings.  That second
path looks like it is addressed later in the series by "vsock: treat
TCP_CLOSING as once-established" and the SS_CONNECTED guard from "vsock:
handle socket bricking due to transport event", but the VMCI detach case
appears to survive to the end of the series.

Would it make sense to make vsock_unconnected_reset() de-assign the
transport unconditionally, or to normalize SOCK_DONE/peer_shutdown there
before the checks?

[Severity: Low]
Could the new precondition be documented?  vsock_assign_transport() is
EXPORT_SYMBOL_GPL'd and called by the transports
(virtio_transport_common.c, vmci_transport.c, hyperv_transport.c), but the
descriptive block above the function still only covers transport selection
and the -ENODEV case:

/* Assign a transport to a socket and call the .init transport callback.
 *
 * Note: for connection oriented socket this must be called when vsk->remote_addr
 * is set ...
 *  - anything else goes to h2g or returns -ENODEV if no h2g is available
 */

and the declaration in include/net/af_vsock.h carries no documentation.  A
note that the function is one-shot per socket and now returns -EINVAL if
vsk->transport or vsk->trans is already set would help future callers.

[ ... ]

> @@ -668,26 +666,6 @@ int vsock_assign_transport(struct vsock_sock *vsk, struct vsock_sock *psk)
>  	 */
>  	mutex_unlock(&vsock_register_mutex);
>  
> -	if (vsk->transport) {
> -		/* transport->release() must be called with sock lock acquired.
> -		 * This path can only be taken during vsock_connect(), where we
> -		 * have already held the sock lock. In the other cases, this
> -		 * function is called on a new socket which is not assigned to
> -		 * any transport.
> -		 */
> -		vsk->transport->release(vsk);
> -		vsock_deassign_transport(vsk);
> -
> -		/* transport's release() and destruct() can touch some socket
> -		 * state, since we are reassigning the socket to a new transport
> -		 * during vsock_connect(), let's reset these fields to have a
> -		 * clean state.
> -		 */
> -		sock_reset_flag(sk, SOCK_DONE);
> -		sk->sk_state = TCP_CLOSE;
> -		WRITE_ONCE(vsk->peer_shutdown, 0);
> -	}
> -

[Severity: Low]
With this block gone, two comments elsewhere in the file still explain a
load-bearing check in terms of reassignment.  Should they be updated?

net/vmw_vsock/af_vsock.c:vsock_remove_sock() {
	/* Transport reassignment must not remove the binding. */
	if (sock_flag(sk_vsock(vsk), SOCK_DEAD))
		vsock_remove_bound(vsk);
}

net/vmw_vsock/af_vsock.c:__vsock_release() {
	/* Indicate to vsock_remove_sock() that the socket is being released and
	 * can be removed from the bound_table. Unlike transport reassignment
	 * case, where the socket must remain bound despite vsock_remove_sock()
	 * being called from the transport release() callback.
	 */
	sock_set_flag(sk, SOCK_DEAD);
}

The SOCK_DEAD test itself still looks necessary, but for other callers:
virtio_transport_recv_connected() and virtio_transport_remove_sock() on the
RST path, and hvs_do_close_lock_held() from hvs_close_connection(), all run
on live sockets without SOCK_DEAD set.  The reassignment reason the
comments name no longer exists after this patch.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-vsock-connect-reset-closing-v3-0-78907b8200d4%40rbox.co

Re: [PATCH net v3 5/8] vsock: treat TCP_CLOSING as once-established

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The failed-connect cleanup at the out_reset label of
  vsock_connect() (net/vmw_vsock/af_vsock.c:1883-1884), which this patch…
- [Low] Concern (validated as NOT a defect to report against this
  series): the new short-circuit in vsock_connect() enumerates only…

--

vsock: treat TCP_CLOSING as once-established

The patch makes blocking connect() stop treating a socket that already
reached TCP_ESTABLISHED and then moved to TCP_CLOSING (or picked up an
sk_err) as a failed connection attempt, returning 0 instead. It also makes
the out_reset cleanup call vsock_unconnected_reset() unconditionally rather
than only when err is non-zero.

Link: https://lore.kernel.org/netdev/anzT1fREOSyHT99k@v4bel/

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 5cecd1c9c6e50..ff4140aaf1f32 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1846,15 +1846,19 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  		timeout = schedule_timeout(timeout);
>  		lock_sock(sk);
>  
> -		/* Connection established. Whatever happens to socket once we
> -		 * release it, that's not connect()'s concern. No need to go
> +		/* Connection was established. Whatever happens to socket once
> +		 * we release it, that's not connect()'s concern. No need to go
>  		 * into signal and timeout handling. Call it a day.
>  		 *
>  		 * Note that allowing to "reset" an already established socket
>  		 * here is racy and insecure.
>  		 */
> -		if (sk->sk_state == TCP_ESTABLISHED)
> -			break;
> +		if (sk->sk_state == TCP_ESTABLISHED ||
> +		    sk->sk_state == TCP_CLOSING) {
> +			err = 0;
> +			finish_wait(sk_sleep(sk), &wait);
> +			goto out;
> +		}

[Severity: Low]
This isn't a bug, but the new short circuit lists only TCP_ESTABLISHED and
TCP_CLOSING as "once established", while an established socket can also be
taken straight to TCP_CLOSE by a transport event while connect() is
scheduled out:

net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
	...
			sk->sk_state = TCP_CLOSE;
		}
		sk->sk_state_change(sk);
}

net/vmw_vsock/virtio_transport.c:virtio_vsock_reset_sock() {
	sk->sk_state = TCP_CLOSE;
	sk->sk_err = ECONNRESET;
	sk_error_report(sk);
}

Those sockets were established as well, but they miss this check, so
connect() still returns -ECONNRESET or -ETIMEDOUT for them.

The cleanup side of that case is covered by the follow-up patch in this
series, "vsock: handle socket bricking due to transport event", which adds
an early return to vsock_unconnected_reset() for sock->state ==
SS_CONNECTED or SS_DISCONNECTING, and neither of the two transport paths
above modifies sk->sk_socket->state.

Is the remaining difference in the connect() return value for the TCP_CLOSE
case the intended behaviour?

>  
>  		/* If connection was _not_ established and a signal/timeout came
>  		 * to be, we want the socket's state reset. We keep the binding.
> @@ -1877,8 +1881,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  out_wait:
>  	finish_wait(sk_sleep(sk), &wait);
>  out_reset:
> -	if (err)
> -		vsock_unconnected_reset(sk);
> +	vsock_unconnected_reset(sk);

[Severity: High]
With this call now unconditional, can a VMCI detach during the handshake
turn this cleanup into a silent no-op plus a warning backtrace?

vmci_transport_handle_detach() sets the flags before it inspects sk_state,
and it explicitly handles the TCP_SYN_SENT case:

net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
	...
		sock_set_flag(sk, SOCK_DONE);
	...
		WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
	...
			if (sk->sk_state == TCP_SYN_SENT) {
				sk->sk_state = TCP_CLOSE;
				sk->sk_err = ECONNRESET;
				sk_error_report(sk);
				return;
			}
}

The valid qp_handle that this path requires is already installed while the
client socket is still TCP_SYN_SENT, by
vmci_transport_recv_connecting_client_negotiate(), which also subscribes
detach_sub_id; TCP_ESTABLISHED is only set later on ATTACH. The detach
callback runs under bh_lock_sock() while blocking connect() has dropped the
socket lock in release_sock()/schedule_timeout().

The woken connect() then sees sk_state == TCP_CLOSE, so the new
ESTABLISHED/TCP_CLOSING short circuit above does not apply, it leaves the
loop on sk_err != 0, takes err = sock_error(sk) and reaches
vsock_unconnected_reset(), where the first check fires and returns:

net/vmw_vsock/af_vsock.c:vsock_unconnected_reset() {
	if (WARN_ON_ONCE(vsk->peer_shutdown) ||
	    WARN_ON_ONCE(sock_flag(sk, SOCK_DONE)) ||
	    WARN_ON_ONCE(sk->sk_state != TCP_SYN_SENT &&
			 sk->sk_state != TCP_CLOSE))
		return;
	...
}

That skips vsock_transport_cancel_pkt(), vsock_deassign_transport(), the
WRITE_ONCE(sk->sk_state, TCP_CLOSE) and sk->sk_socket->state =
SS_UNCONNECTED.

Does that leave the socket with its transport still assigned and
sock->state stuck at SS_CONNECTING, so later connect() attempts return
-EALREADY for O_NONBLOCK, or re-enter the loop, burn the full
vsk->connect_timeout and return -ETIMEDOUT?

Before this series the same path simply did sk->sk_state = TCP_CLOSE;
sock->state = SS_UNCONNECTED; with no warning, and the socket stayed
retryable. At the end of the series the new SS_CONNECTED/SS_DISCONNECTING
early return in vsock_unconnected_reset() does not cover this case, since
sock->state is SS_CONNECTING here and sk_state is TCP_CLOSE, so the
WARN_ON_ONCE(vsk->peer_shutdown) check is still the one that fires.

>  out:
>  	release_sock(sk);
>  	return err;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-vsock-connect-reset-closing-v3-0-78907b8200d4%40rbox.co

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Over-broad new guard in vsock_unconnected_reset(): the early
  return also fires for sockets that were never established.
- [High] Incomplete race guard: vmci_transport_handle_detach() can brick
  a socket that is still TCP_SYN_SENT / SS_CONNECTING, which the new…

--

vsock: handle socket bricking due to transport event

Transport events reset connected sockets to TCP_CLOSE without holding
lock_sock(), so connect() can observe the reset concurrently.  This patch
makes vsock_unconnected_reset() bail out early when the socket-level state
is SS_CONNECTED or SS_DISCONNECTING, on the premise that transport events
leave SS_CONNECTED unchanged.

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

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?

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

vmci_transport_handle_detach() runs from vmci_transport_peer_detach_cb()
under trans->lock plus bh_lock_sock() only, i.e. concurrently with a
connect() that released the socket lock in schedule_timeout(), and it
mutates a socket that is still in the handshake:

net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
	if (!vmci_handle_is_invalid(vmci_trans(vsk)->qp_handle)) {
		sock_set_flag(sk, SOCK_DONE);
		WRITE_ONCE(vsk->peer_shutdown, SHUTDOWN_MASK);
		...
			if (sk->sk_state == TCP_SYN_SENT) {
				sk->sk_state = TCP_CLOSE;
				sk->sk_err = ECONNRESET;
	...
}

A client socket already owns a valid qp_handle while still TCP_SYN_SENT,
because vmci_transport_recv_connecting_client_negotiate() publishes
vmci_trans(vsk)->qp_handle and detach_sub_id before ATTACH arrives;
SS_CONNECTED is only set later in the ATTACH branch of
vmci_transport_recv_connecting_client().

Since the handler never touches sk->sk_socket->state, connect() left it at
SS_CONNECTING, so the new check does not match.  connect()'s loop exits on
sk_err != 0 and reaches out_reset -> vsock_unconnected_reset(), where the
existing checks below fire:

	if (WARN_ON_ONCE(vsk->peer_shutdown) ||
	    WARN_ON_ONCE(sock_flag(sk, SOCK_DONE)) ||
	    WARN_ON_ONCE(sk->sk_state != TCP_SYN_SENT &&
			 sk->sk_state != TCP_CLOSE))
		return;

Can a peer VM detaching mid-handshake therefore still produce these
WARN_ON_ONCE() splats and still skip vsock_transport_cancel_pkt() and
vsock_deassign_transport(), leaving the stale transport assignment the
patch aims to avoid?

This appears to be still present at the end of the series; the later commit
6b7829ad only adds the TCP_LISTEN check.

>  	/*
>  	 * Only connected socks may have peer_shutdown or SOCK_DONE set.
>  	 *

Thanks for looking into these.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-vsock-connect-reset-closing-v3-0-78907b8200d4%40rbox.co

Re: [PATCH net v3 7/8] vsock: handle listen() racing connect()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message states the goal is to 'let connect() time
  out, but don't reset the socket', implying the pre-patch code resets a…
- [Low] The patch fixes a user-visible, unprivileged-reachable defect
  that exists in released kernels but carries no `Fixes:` tag and no…

--

vsock: handle listen() racing connect()

This adds an early return to vsock_unconnected_reset() so that a socket
which a racing listen() has moved to TCP_LISTEN is left alone. A blocking
connect() that was sleeping in schedule_timeout() then returns its error
without demoting the listener.

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 5a2d7e10ecb88..56c5959424564 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1663,6 +1663,13 @@ static void vsock_unconnected_reset(struct sock *sk)
>  
>  	sock_owned_by_me(sk);
>  
> +	/*
> +	 * listen() can race connect() on schedule_timeout().
> +	 * Don't reset TCP_LISTEN.
> +	 */
> +	if (sk->sk_state == TCP_LISTEN)
> +		return;
> +

[Severity: Medium]
The changelog says the intent is to "let connect() time out, but don't
reset the socket", which reads as though the code just before this patch
resets a TCP_LISTEN socket.

Is that still true with the immediate parent ("vsock: handle socket
bricking due to transport event") applied?  In that tree the race
described in the changelog reaches the checks already present in
vsock_unconnected_reset():

net/vmw_vsock/af_vsock.c:vsock_unconnected_reset() {
	...
	if (WARN_ON_ONCE(vsk->peer_shutdown) ||
	    WARN_ON_ONCE(sock_flag(sk, SOCK_DONE)) ||
	    WARN_ON_ONCE(sk->sk_state != TCP_SYN_SENT &&
			 sk->sk_state != TCP_CLOSE))
		return;
	...
}

With sk_state == TCP_LISTEN and sk->sk_socket->state == SS_UNCONNECTED,
the SS_CONNECTED/SS_DISCONNECTING guard does not fire, peer_shutdown is
zero and SOCK_DONE is unset, so control reaches the third WARN_ON_ONCE
and returns without resetting anything.

That makes the pre-patch symptom at this point of the series a
userspace-triggerable splat from vsock_connect()'s out_reset: path rather
than a lost listener, and a splat is fatal with panic_on_warn.  The
window covers the commits from "vsock: do not preserve transport
assignment" through "vsock: handle socket bricking due to transport
event", so a bisect or a partial backport can land on it.

Would it make sense to squash this TCP_LISTEN check into the commit that
introduced that WARN_ON_ONCE, or otherwise mention the warning in the
changelog?

[Severity: Low]
Should this patch carry a Fixes: tag?  The sibling fixes in the same
series carry Fixes: d021c344051a ("VSOCK: Introduce VM Sockets"), while
this one has only a Signed-off-by.

Released kernels appear to hit the same race without this guard.  At the
pre-series baseline, vsock_connect() writes the state back unconditionally
with no sk_state test:

net/vmw_vsock/af_vsock.c:vsock_connect() {
	...
	err = sock_error(sk);
	if (err) {
		sk->sk_state = TCP_CLOSE;
		sock->state = SS_UNCONNECTED;
	}
	...
}

and vsock_listen() only requires SS_UNCONNECTED plus a bound address
before flipping the state:

net/vmw_vsock/af_vsock.c:vsock_listen() {
	...
	if (sock->state != SS_UNCONNECTED) {
		err = -EINVAL;
		goto out;
	}
	...
	if (!vsock_addr_bound(&vsk->local_addr)) {
		err = -EINVAL;
		goto out;
	}

	sk->sk_max_ack_backlog = backlog;
	sk->sk_state = TCP_LISTEN;
	...
}

So on a stable tree, two threads sharing one fd (non-blocking connect()
arming vsock_connect_timeout(), blocking connect() sleeping in
schedule_timeout(), then listen()) can end up with the listener demoted
to TCP_CLOSE/SS_UNCONNECTED and accept() returning -EINVAL.  Without a
Fixes: or Cc: stable trailer, is there anything to signal that to
backporters?

>  	/*
>  	 * Transport events are serviced by resetting each connected socket to
>  	 * TCP_CLOSE. The reset is done under vsock_table_lock but without

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922-vsock-connect-reset-closing-v3-0-78907b8200d4%40rbox.co

Re: [PATCH net v3 3/8] vsock: do not preserve transport assignment

[Michal Luczaj]

On 9/24/26 15:15, netdev-bot+sashiko@kernel.org wrote:
>> +static void vsock_unconnected_reset(struct sock *sk)
>> +{
>> +	struct vsock_sock *vsk = vsock_sk(sk);
>> +
>> +	sock_owned_by_me(sk);
>> +
>> +	/*
>> +	 * Only connected socks may have peer_shutdown or SOCK_DONE set.
>> +	 *
>> +	 * Once established (TCP_ESTABLISHED, TCP_CLOSING), a socket can be
>> +	 * de-assigned only on close(). But we can narrow the check down to
>> +	 * states we actually expect (TCP_SYN_SENT, TCP_CLOSE).
>> +	 */
>> +	if (WARN_ON_ONCE(vsk->peer_shutdown) ||
>> +	    WARN_ON_ONCE(sock_flag(sk, SOCK_DONE)) ||
>> +	    WARN_ON_ONCE(sk->sk_state != TCP_SYN_SENT &&
>> +			 sk->sk_state != TCP_CLOSE))
>> +		return;
> 
> [Severity: High]
> Is the "only connected socks may have peer_shutdown or SOCK_DONE set"
> invariant true for VMCI?
> 
> vmci_transport_recv_connecting_client_negotiate() allocates the queue pair
> and subscribes to VMCI_EVENT_QP_PEER_DETACH while the socket is still in
> TCP_SYN_SENT. If the peer then detaches, vmci_transport_handle_detach()
> runs on a never-established socket:
> 
> The blocking connect() waiter then wakes on sk_err, err = sock_error(sk)
> becomes -ECONNRESET, and out_reset calls this helper with peer_shutdown ==
> SHUTDOWN_MASK and SOCK_DONE set.
> 
> Since WARN_ON_ONCE() evaluates to the condition, the helper splats and
> returns before vsock_transport_cancel_pkt(), vsock_deassign_transport() and
> the TCP_CLOSE/SS_UNCONNECTED restore. Can that leave the socket stuck with
> sk_socket->state == SS_CONNECTING and the transport (plus its module
> reference) pinned, so a later connect() returns -EALREADY for O_NONBLOCK or
> re-hits the same early return, and listen() returns -EINVAL?
> 
> Before this patch the same path unconditionally ran:
> 
> 	err = sock_error(sk);
> 	if (err) {
> 		sk->sk_state = TCP_CLOSE;
> 		sock->state = SS_UNCONNECTED;
> 	}
> 
> so the socket stayed retryable. Is the change in behaviour here intended?

True, I'll drop WARN_ON_ONCE(vsk->peer_shutdown).

> On panic_on_warn=1 kernels this is also reachable from an unprivileged
> connect() when the peer VM goes away mid-handshake.

It was meant for debugging and to trip the fuzzers. Would it be better to
use DEBUG_NET_WARN_ON_ONCE?

> [Severity: High]
> Can this de-assign the transport of a socket that did reach
> TCP_ESTABLISHED?
> ...
> A later patch in this series, "vsock: handle socket bricking due to
> transport event", adds an early return on sk_socket->state == SS_CONNECTED
> or SS_DISCONNECTING at the top of this helper, which covers exactly this
> case, so this only affects this commit taken on its own. Would it be
> cleaner to fold that guard in here?

Maybe it would be cleaner, but I think it's easier to review as it is now.

> [Severity: Medium]
> For a non-blocking connect() whose failure is detected asynchronously by
> the transport, which path reaches out_reset?
> 
> connect(fd, O_NONBLOCK) assigns a transport, sets sk_state = TCP_SYN_SENT,
> sock->state = SS_CONNECTING and returns -EINPROGRESS. If the peer answers
> with VIRTIO_VSOCK_OP_RST, virtio_transport_recv_connecting() takes its
> destroy label:
> 
> net/vmw_vsock/virtio_transport_common.c:virtio_transport_recv_connecting() {
> 	...
> destroy:
> 	virtio_transport_reset(vsk, skb);
> 	sk->sk_state = TCP_CLOSE;
> 	sk->sk_err = skerr;
> 	sk_error_report(sk);
> 	...
> }
> 
> sk_socket->state is left at SS_CONNECTING and the transport stays assigned.
> vmci_transport_recv_connecting_client() has the same pattern.
> 
> vsock_connect_timeout() above is gated on sk->sk_state == TCP_SYN_SENT, so
> it does not reset either once the state is TCP_CLOSE. And a retry with
> O_NONBLOCK hits:
> 
> 	case SS_CONNECTING:
> 		...
> 		err = -EALREADY;
> 		if (flags & O_NONBLOCK)
> 			goto out;
> 
> which returns before out_reset.
> 
> Does the socket then keep the transport assignment and the transport module
> reference until close()? The changelog says:
> 
>     If connection fails (init went wrong, peer misbehaviour, time out,
>     signal), transport is de-assigned and socket state is re-initialized.
> 
> Should the asynchronous peer-misbehaviour case for a non-blocking connect()
> be covered as well, or should the changelog be narrowed to the synchronous
> paths?
OK, so a _non-blocking_ connect() getting hit with RST before connection
can be established: fd bricked, you can't re-connect() with it. While you
can retry a _blocking_ connect getting hit the same way. And the choice is
to unify the behaviour or document the discrepancy, if I get it right.

Re: [PATCH net v3 4/8] vsock: drop the re-assignment logic

[Michal Luczaj]

On 9/24/26 15:15, netdev-bot+sashiko@kernel.org wrote:
> [Severity: Medium]
> Is the invariant behind this assertion always true?  The commit message
> says "On a failed connect(), transport is de-assigned", but
> vsock_unconnected_reset() can return early without de-assigning:

Addressed in comments to patch #3.

> [Severity: Low]
> Could the new precondition be documented?

Yes.

> [Severity: Low]
> With this block gone, two comments elsewhere in the file still explain a
> load-bearing check in terms of reassignment.  Should they be updated?

Yes.

Re: [PATCH net v3 5/8] vsock: treat TCP_CLOSING as once-established

[Michal Luczaj]

On 9/24/26 15:15, netdev-bot+sashiko@kernel.org wrote:
> [Severity: Low]
> This isn't a bug, but the new short circuit lists only TCP_ESTABLISHED and
> TCP_CLOSING as "once established", while an established socket can also be
> taken straight to TCP_CLOSE by a transport event while connect() is
> scheduled out:
> ...
> The cleanup side of that case is covered by the follow-up patch in this
> series, "vsock: handle socket bricking due to transport event", which adds
> an early return to vsock_unconnected_reset() for sock->state ==
> SS_CONNECTED or SS_DISCONNECTING, and neither of the two transport paths
> above modifies sk->sk_socket->state.
> 
> Is the remaining difference in the connect() return value for the TCP_CLOSE
> case the intended behaviour?

Should I use a specific return value for a socket that got bricked by
transport event?

>> @@ -1877,8 +1881,7 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>>  out_wait:
>>  	finish_wait(sk_sleep(sk), &wait);
>>  out_reset:
>> -	if (err)
>> -		vsock_unconnected_reset(sk);
>> +	vsock_unconnected_reset(sk);
> 
> [Severity: High]
> With this call now unconditional, can a VMCI detach during the handshake
> turn this cleanup into a silent no-op plus a warning backtrace?
Addresses in comments to patch #3.

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

[Michal Luczaj]

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.

Re: [PATCH net v3 7/8] vsock: handle listen() racing connect()

[Michal Luczaj]

On 9/24/26 15:15, netdev-bot+sashiko@kernel.org wrote:
> [Severity: Medium]
> ...
> Would it make sense to squash this TCP_LISTEN check into the commit that
> introduced that WARN_ON_ONCE, or otherwise mention the warning in the
> changelog?

I think it's easier to review as it is now.

> [Severity: Low]
> Should this patch carry a Fixes: tag?  The sibling fixes in the same
> series carry Fixes: d021c344051a ("VSOCK: Introduce VM Sockets"), while
> this one has only a Signed-off-by.

Yes.