Re: [PATCH net] vsock: Do not reset a TCP_CLOSING socket

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Wed, Sep 09, 2026 at 11:58:26PM +0200, Michal Luczaj wrote:
> Ensure connect() resets the socket only if it has never been established.
> Handle the previously overlooked TCP_ESTABLISHED -> TCP_CLOSING
> transition (on VIRTIO_VSOCK_OP_RST), which could race with the connect
> loop.
> 
> 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
> 
> Drop the inaccurate comment above signal_pending(). This fix is
> supplementary to commit 002541ef650b ("vsock: Ignore signal/timeout on
> connect() if already established"). Details at 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>
> ---
> Note that this is not a complete fix. connect()'s schedule_timeout() can
> still race with two other functions that set sk_state = TCP_CLOSE while
> keeping the socket in connected_table:
> 1. vmci_transport_handle_detach(): no way for me to test,
> 2. virtio_vsock_reset_sock(): tested by unbinding the driver
>    (/sys/bus/virtio/drivers/virtio_transport/unbind).
> The latter appears easy to fix by adding __vsock_remove_connected() and
> switching to a _safe iterator in vsock_for_each_connected_socket().
> ---
>  net/vmw_vsock/af_vsock.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index f840498b58af..eec5dd6daebb 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -1834,23 +1834,20 @@ 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)
> +		if (sk->sk_state == TCP_ESTABLISHED ||
> +		    sk->sk_state == TCP_CLOSING)
>  			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.
>  		 */
>  		if (signal_pending(current) || timeout == 0) {
>  			err = timeout == 0 ? -ETIMEDOUT : sock_intr_errno(timeout);
> @@ -1875,7 +1872,8 @@ static int vsock_connect(struct socket *sock, struct sockaddr_unsized *addr,
>  	}
>  
>  	err = sock_error(sk);
> -	if (err) {
> +	if (err &&
> +	    sk->sk_state != TCP_ESTABLISHED && sk->sk_state != TCP_CLOSING) {

If the OP_RESPONSE + a blast of OP_RW that pushes past the buffer limit
arrives while we were scheduled out, we end up with sk_err = ENOBUFS
here. Then I guess connect() returns an error, but sk_state/sock->state
is still TCP_ESTABLISHED and SS_CONNECTED. If the user sees the error
and tries connect() again, they just get -EISCONN back. Maybe the
sock_error() needs to be moved within the conditional here, and then let
subsequent calls return the error to the user (it looks sendmsg() at
least will report it faithfully, but not sure about recvmsg() or the
others).

Best,
Bobby