[PATCH net v4] vsock: fix connect() races

[PATCH net v4] vsock: fix connect() races

[Michal Luczaj]

Handle a racy socket change in connect(). This iteration focuses on a
single issue and aims for minimal changes.

I'm keeping this as a 1-patch series so as not to confuse the patchwork.

---
Changes in v4:
- Defer all other patches to net-next (Stefano)
- Link to v3: https://patch.msgid.link/20260922-vsock-connect-reset-closing-v3-0-78907b8200d4@rbox.co

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 (1):
      vsock: treat TCP_CLOSING as once-established

 net/vmw_vsock/af_vsock.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)
---
base-commit: 6dc989ea46b96ce170840174b4a38c4a387fb005
change-id: 20260820-vsock-connect-reset-closing-98dd28769d7f

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

[PATCH net v4] vsock: treat TCP_CLOSING as once-established

[Michal Luczaj]

Handle the TCP_ESTABLISHED -> TCP_CLOSING transition on OP_RST, which can
race with the connect loop. Immediately break and return 0 on
ESTABLISHED/CLOSING. The return value of connect() should reflect what
happened up to the point the connection was established or failed. Events
that occur afterwards must not affect it. E.g. even if OP_RW has already
set sk_err, connect() should still return 0, not ENOBUFS because of it.
Adapt the inaccurate comment above signal_pending().

Resetting a socket that is still present in connected_table can lead to
memory corruption. The reporter noted lost transports for in-flight skbs,
and I have reproduced crashes caused by re-insertion into connected_table.

  list_add double add: new=, prev=, next=.
  kernel BUG at lib/list_debug.c:35!
  Oops: invalid opcode: 0000 [#1] SMP KASAN NOPTI
  Workqueue: vsock-loopback vsock_loopback_work
  RIP: 0010:__list_add_valid_or_report+0x11f/0x130
  Call Trace:
   vsock_insert_connected.cold+0xe/0x13
   virtio_transport_recv_pkt+0x10e9/0x1460
   vsock_loopback_work+0x305/0x480
   process_one_work+0xe4c/0x1560
   worker_thread+0x4f1/0xd60
   kthread+0x36e/0x470
   ret_from_fork+0x47b/0x6b0
   ret_from_fork_asm+0x1a/0x30

This fix is supplementary to commit 002541ef650b ("vsock: Ignore
signal/timeout on connect() if already established"). Details under Link.

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 | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index 9b71479a2b29..14fe24b05f9c 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1834,23 +1834,22 @@ 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;
+			goto out_wait;
+		}
 
 		/* 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 retry, so we keep the binding and the transport assigned.
 		 */
 		if (signal_pending(current) || timeout == 0) {
 			err = timeout == 0 ? -ETIMEDOUT : sock_intr_errno(timeout);

-- 
2.56.0

Re: [PATCH net v4] vsock: treat TCP_CLOSING as once-established

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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] The fix is incomplete.

--

vsock: treat TCP_CLOSING as once-established

This patch makes vsock_connect() return 0 right away when the socket is
found in TCP_ESTABLISHED or TCP_CLOSING after waking from
schedule_timeout(). Before, only TCP_ESTABLISHED was checked. The aim is
to stop connect() from resetting a socket that is still in
vsock_connected_table, which can lead to a double insertion and list
corruption.

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 9b71479a2b295..14fe24b05f9ce 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1834,23 +1834,22 @@ 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;
> +			goto out_wait;
> +		}

[Severity: High]
Can vsock_connect() still reset a socket that is already linked in
vsock_connected_table? This check only covers TCP_ESTABLISHED and
TCP_CLOSING. At least two transports move an established socket that is
already in the table straight to TCP_CLOSE, and neither one unlinks it.

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

virtio_vsock_event_handle() calls this for every connected socket on
VIRTIO_VSOCK_EVENT_TRANSPORT_RESET. It does not take lock_sock().

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

This runs from vmci_transport_peer_detach_cb() with only bh_lock_sock()
held and no sock_owned_by_user() check. For a socket that is not in
TCP_SYN_SENT, it leaves sk_err at 0.

Suppose either of these runs after the RESPONSE/ATTACH handler has set
TCP_ESTABLISHED and SS_CONNECTED and called vsock_insert_connected(), but
before the sleeping connect() thread takes the lock again:

  connect() thread               RX / event path
  schedule_timeout()
                                 sk_state = TCP_ESTABLISHED
                                 vsock_insert_connected(vsk)
                                 virtio_vsock_reset_sock() or
                                 vmci_transport_handle_detach()
                                   sk_state = TCP_CLOSE
  lock_sock()
  sk_state == TCP_CLOSE, new check does not match

For virtio, sk_err != 0 ends the loop, and then the reset after the loop
runs:

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

For VMCI, sk_err stays 0. The loop keeps waiting until a signal arrives or
the timeout expires, and then the signal/timeout branch sets TCP_CLOSE and
SS_UNCONNECTED.

In both cases connect() returns an error for a connection that was
established. The socket is also left SS_UNCONNECTED while it is still
linked in vsock_connected_table and still holds the table's reference.

If user space then retries connect(), vsock_assign_transport() returns
early because the transport is the same:

	if (vsk->transport && vsk->transport == new_transport) {
		ret = 0;
		goto err;
	}

so vsock_remove_sock() never runs and the stale table entry survives.

For virtio, the next OP_RESPONSE goes through
virtio_transport_recv_connecting(), which calls vsock_insert_connected()
again. For VMCI, an ATTACH that carries the kept qp_handle does the same
thing in vmci_transport_recv_connecting_client(). In the VMCI case the
detach and the later ATTACH both come from the peer.

__vsock_insert_connected() calls sock_hold() and list_add() without
checking whether the socket is already in the list. Wouldn't this hit the
same "list_add double add" BUG quoted in the commit message, and also leak
a socket reference?

sk_state does not seem to track table membership reliably. Would it be
more robust to check sock->state == SS_CONNECTED instead (the transports
set it together with vsock_insert_connected()), or to check membership in
connected_table directly? That check would have to guard both the
signal/timeout reset inside the loop and the sock_error() reset after the
loop.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005-vsock-connect-reset-closing-v4-1-15d41cfda3ed%40rbox.co

Re: [PATCH net v4] vsock: treat TCP_CLOSING as once-established

[Michal Luczaj]

On 10/7/26 02:27, netdev-bot+sashiko@kernel.org wrote:
> [Severity: High]
> Can vsock_connect() still reset a socket that is already linked in
> vsock_connected_table? This check only covers TCP_ESTABLISHED and
> TCP_CLOSING. At least two transports move an established socket that is
> already in the table straight to TCP_CLOSE, and neither one unlinks it.
> 
> net/vmw_vsock/virtio_transport.c:virtio_vsock_reset_sock() {
> 	sk->sk_state = TCP_CLOSE;
> 	sk->sk_err = ECONNRESET;
> 	sk_error_report(sk);
> }
> 
> virtio_vsock_event_handle() calls this for every connected socket on
> VIRTIO_VSOCK_EVENT_TRANSPORT_RESET. It does not take lock_sock().
> 
> net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
> 	...
> 			sk->sk_state = TCP_CLOSE;
> 		}
> 		sk->sk_state_change(sk);
> 	...
> }

Right, that's a known issue[1], to be handled separately.

> [ ... ]
> sk_state does not seem to track table membership reliably. Would it be
> more robust to check sock->state == SS_CONNECTED instead (the transports
> set it together with vsock_insert_connected()),

vsock_shutdown() complicates this. It can flip SS_CONNECTED to
SS_DISCONNECTING.

> or to check membership in
> connected_table directly? That check would have to guard both the
> signal/timeout reset inside the loop and the sock_error() reset after the
> loop.

Lack of membership check for connected_table is not the root cause of the
issue. But, sure, we can discuss adding it as a sanity check.

[1]: https://lore.kernel.org/netdev/arpBYXWb9pohx-8v@sgarzare-redhat/