Re: [PATCH net] vhost/vsock: Fix socket state constant

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] vhost/vsock: vhost_vsock_reset_orphans() TCP_CLOSE breaks
  vsock_connect()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] vhost/vsock: data race in vhost_vsock_reset_orphans() [1]
--

commit 5aff0946f70c88424d509989526e2bc91c501e
Author: Michal Luczaj <mhal@rbox.co>

vhost/vsock: Fix socket state constant

This commit updates vhost_vsock_reset_orphans() to set sk->sk_state to
TCP_CLOSE instead of SS_UNCONNECTED when resetting orphaned connections.

> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index 88a310390eb0a..fd224f7681d92 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -789,7 +789,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);
>  }

[Severity: High]
Can setting sk->sk_state to TCP_CLOSE here cause vsock_connect() to reset
sock->state to SS_UNCONNECTED while vsk remains in vsock_connected_table?

 Previously, SS_UNCONNECTED (1) happened to match TCP_ESTABLISHED (1), so
vsock_connect() treated the socket as once-established.  With TCP_CLOSE, a
vhost device release racing with a completing connection can hit the error
path in vsock_connect():

CPU0                              CPU1
vsock_connect()
  release_sock(sk)
  schedule_timeout()
                                  virtio_transport_recv_connecting()
                                    sk->sk_state = TCP_ESTABLISHED
                                    sock->state = SS_CONNECTED
                                    vsock_insert_connected(vsk)
                                  vhost_vsock_dev_release()
                                    ...
                                      vhost_vsock_reset_orphans(sk)
                                        sk->sk_state = TCP_CLOSE
                                        sk->sk_err = ECONNRESET
  lock_sock(sk)
  if (sk->sk_state == TCP_ESTABLISHED ||
      sk->sk_state == TCP_CLOSING)        <-- false
  ...
  err = sock_error(sk)
  if (err)
    sock->state = SS_UNCONNECTED          <-- still in vsock_connected_table

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

Because sock->state is reset to SS_UNCONNECTED while vsk is still linked in
vsock_connected_table, a subsequent connect() on the same socket can
reassign its transport or, if shutdown() clears SOCK_DONE and a new
VIRTIO_VSOCK_OP_RESPONSE arrives, call vsock_insert_connected(vsk) a second
time and corrupt vsock_connected_table.  Setting sk->sk_state to
TCP_CLOSING instead (or checking sock_flag(sk, SOCK_DONE) in
vsock_connect()) would preserve the once-established check.

[Severity: High]
This problem wasn't introduced by this patch, but vhost_vsock_reset_orphans()
modifies sk->sk_flags, vsk->peer_shutdown, sk->sk_state, and sk->sk_err
under vsock_table_lock without holding lock_sock(sk) [1].  Concurrent
socket operations such as vsock_connect(), sendmsg(), or recvmsg() holding
the socket lock can race with these updates and observe inconsistent socket
state.

[1] https://sashiko.dev/#/bug/linux-f1ccca0e-e7ae-4926-a24d-72f61680c524

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112-transport-reset-state-fix-v1-1-5698375f52a9@rbox.co?part=1