Re: [PATCH net-next v6 1/8] vsock: only undo SS_CONNECTING when connect() fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] vsock: vsock_connect() fails on SS_CONNECTED sockets reset to
  TCP_CLOSE
- [Medium] vsock: vsock_connect_close() sets TCP_CLOSE when state !=
  SS_CONNECTING
--

commit 02952dc6bba9fe20e7c14b7fd24b6a5865ca1aab
Author: Bobby Eshleman <bobbyeshleman@meta.com>

vsock: only undo SS_CONNECTING when connect() fails

This commit adds vsock_connect_close() so that a failed vsock_connect()
only resets sock->state to SS_UNCONNECTED if it is still SS_CONNECTING,
leaving sockets that became SS_CONNECTED while sleeping in that state.

Link: https://lore.kernel.org/netdev/179447093204.2199510.8895677421994612994@kernel.org/

> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index 483af15a5109e..dc81cd47f099d 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1703,6 +1703,16 @@ static void vsock_connect_timeout(struct work_struct *work)
>  	sock_put(sk);
>  }
>  
> +/* Avoid breaking the socket when another thread has changed its sock->state
> + * while we were asleep.
> + */
> +static void vsock_connect_close(struct socket *sock)
> +{
> +	sock->sk->sk_state = TCP_CLOSE;
> +	if (sock->state == SS_CONNECTING)
> +		sock->state = SS_UNCONNECTED;
> +}

[Severity: Medium]
Can vsock_connect_close() overwrite sk->sk_state with TCP_CLOSE after
another thread has moved the socket out of SS_CONNECTING?

Because vsock_connect() allows a second blocking caller into the wait loop
when sock->state is SS_CONNECTING, two threads can sleep in
schedule_timeout() on the same socket:

CPU0                                  CPU1
vsock_connect()
  sock->state = SS_CONNECTING
  schedule_timeout()
                                      vsock_connect()
                                        case SS_CONNECTING:
                                        schedule_timeout()
  <connect fails>
  err = sock_error(sk)
  vsock_connect_close(sock)
    sk->sk_state = TCP_CLOSE
    sock->state = SS_UNCONNECTED
vsock_listen()
  sk->sk_state = TCP_LISTEN
                                        <timeout or signal>
                                        vsock_connect_close(sock)
                                          sk->sk_state = TCP_CLOSE

When CPU0 consumes sk->sk_err via sock_error(sk), CPU1 stays in the wait
loop because sk->sk_state is TCP_CLOSE and sk->sk_err is 0.  If CPU0 then
calls vsock_listen() (which leaves sock->state as SS_UNCONNECTED and sets
sk->sk_state to TCP_LISTEN), CPU1 waking on a timeout or signal calls
vsock_connect_close() and clobbers TCP_LISTEN with TCP_CLOSE, allowing a
subsequent vsock_connect() to bypass the sk->sk_state == TCP_LISTEN check.

> @@ -1861,8 +1871,7 @@ 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;
> +			vsock_connect_close(sock);
>  
>  			/* Try to cancel VIRTIO_VSOCK_OP_REQUEST skb sent out by
>  			 * transport->connect().
> @@ -1876,10 +1885,8 @@ 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;
> -	}
> +	if (err)
> +		vsock_connect_close(sock);

[Severity: Medium]
What happens in vsock_connect() if the handshake completes (setting
sock->state to SS_CONNECTED) and the connection is then reset to TCP_CLOSE
while schedule_timeout() is sleeping?

Both virtio_vsock_reset_sock() and vmci_transport_handle_detach() move an
established socket directly to TCP_CLOSE rather than TCP_CLOSING:

net/vmw_vsock/vmci_transport.c:vmci_transport_handle_detach() {
    ...
		} else {
			if (sk->sk_state == TCP_SYN_SENT) {
				sk->sk_state = TCP_CLOSE;
				sk->sk_err = ECONNRESET;
				sk_error_report(sk);
				return;
			}
			sk->sk_state = TCP_CLOSE;
    ...
}

When vsock_connect() reacquires lock_sock(sk), the check for
sk->sk_state == TCP_ESTABLISHED || sk->sk_state == TCP_CLOSING misses
this socket because sk->sk_state is already TCP_CLOSE.

If vmci_transport_handle_detach() ran on an established socket (leaving
sk->sk_err == 0), or if another thread consumed sk->sk_err via
getsockopt(SO_ERROR) or sendmsg(), the wait loop condition
(sk->sk_state != TCP_ESTABLISHED && sk->sk_state != TCP_CLOSING &&
sk->sk_err == 0) remains true and vsock_connect() sleeps until
connect_timeout expires or a signal arrives.  It then enters the
timeout/signal path, leaves sock->state as SS_CONNECTED via
vsock_connect_close(), and calls vsock_transport_cancel_pkt(vsk).  If a
concurrent sendmsg() queued VIRTIO_VSOCK_OP_RW skbs while the socket was
connected, purging them via virtio_transport_purge_skbs() without calling
virtio_transport_consume_skb_sent() leaks vvs->bytes_unsent, breaking
SIOCOUTQ and causing vsock_linger() to wait on close.  Returning
-ERESTARTSYS on a signal while leaving sock->state as SS_CONNECTED also
causes a restarted connect() syscall to fail with -EISCONN.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-vsock-guest-ns-v6-0-3737ce4673c0@meta.com?part=1