Re: [Devel] [PATCH vz10 3/5] fixup! vhost/vsock: only refuse connection when guest has never been ready

[Konstantin Khorenko <khorenko@virtuozzo.com>]

On 6/25/26 20:16, Eva Kurchatova wrote:
> From: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> 
> Commit 4ff28534c799 ("ms/vhost/vsock: Refuse the connection immediately
> when guest isn't ready") added a check which immediately returns
> EHOSTUNREACH when the guest isn't ready yet.  Namely, we check that guest
> hasn't enabled the RX vq yet, i.e. virtio-vsock has never beed enabled.

s/beed/been/

> 
> However, the check also affects the transient state when backend is
> temporarily set to NULL during VHOST_VSOCK_SET_RUNNING(0).  Notably,
> this is the case with qemu-update operation, during which we perform
> VHOST_RESET_OWNER.  In this case sendmsg()/connect() on otherwise healthy
> connection gets EHOSTUNREACH.
> 
> Gate the fast-fail on a sticky started_once bit set in
> vhost_vsock_start() and never cleared.  Once the guest has brought
> up virtio-vsock at least once, a NULL backend means a transient stop
> window and the packet must be queued for vhost_vsock_start() to drain
> on re-attach.
> 
> Fixes: 4ff28534c799 ("ms/vhost/vsock: Refuse the connection immediately when guest isn't ready")
> https://virtuozzo.atlassian.net/browse/VSTOR-131956
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  drivers/vhost/vsock.c | 31 ++++++++++++++-----------------
>  1 file changed, 14 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> index d4c3f94308db..f652f47956d7 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -59,6 +59,7 @@ struct vhost_vsock {
>  
>  	u32 guest_cid;
>  	bool seqpacket_allow;
> +	bool started_once;	/* latched in vhost_vsock_start(); never cleared */
>  	bool cpr_paused;	/* between stop and next start; queues sends */
>  };

static int vhost_vsock_dev_open(struct inode *inode, struct file *file)
{
...
        vsock = kvmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL);

So, not zeroing struct fields here on allocation.
.started_once is not directly initialized => can have any start value.

>  
> @@ -289,24 +290,17 @@ vhost_transport_send_pkt(struct sk_buff *skb, struct net *net)
>  		return -ENODEV;
>  	}
>  
> -	/* Fast-fail if the guest hasn't enabled the RX vq yet. Queuing the packet
> -	 * and making the caller wait is pointless: even if the guest manages to init
> -	 * within the timeout, it'll immediately reply with RST, because there's no
> -	 * listener on the port yet.
> -	 *


> -	 * vhost_vq_get_backend() without vq->mutex is acceptable here: locking
> -	 * the mutex would be too expensive in this hot path, and we already have
> -	 * all the outcomes covered: if the backend becomes NULL right after the check,
> -	 * vhost_transport_do_send_pkt() will check it under the mutex anyway.

Why you drop this part of the comment? It looks useful.

> +	/*
> +	 * Fast-fail only when the guest has never enabled virtio-vsock.
> +	 * Once it has, a NULL backend means a transient SET_RUNNING(0)
> +	 * window (e.g. VHOST_RESET_OWNER); the packet must be
> +	 * queued for vhost_vsock_start() to drain on re-attach.
>  	 */
> -	/* cpr_paused: queue across CPR; else NULL backend means not ready. */
> -	if (unlikely(!data_race(vhost_vq_get_backend(&vsock->vqs[VSOCK_VQ_RX])))) {
> -		smp_rmb();	/* pairs with smp_wmb() in start/drop_backends */
> -		if (!READ_ONCE(vsock->cpr_paused)) {

(kostja at f0)/git/vzkernel.vz10:git grep cpr_paused
drivers/vhost/vsock.c:  bool cpr_paused;        /* between stop and next start; queues sends */
drivers/vhost/vsock.c:  /* cpr_paused: queue across CPR; else NULL backend means not ready. */
drivers/vhost/vsock.c:          if (!READ_ONCE(vsock->cpr_paused)) {
drivers/vhost/vsock.c:  WRITE_ONCE(vsock->cpr_paused, false);
drivers/vhost/vsock.c:          WRITE_ONCE(vsock->cpr_paused, true);
drivers/vhost/vsock.c:  vsock->cpr_paused = false;

So you are dropping the only READ of this vsock->cpr_paused, so it's not needed anymore after this patch.

> -			rcu_read_unlock();
> -			kfree_skb(skb);
> -			return -EHOSTUNREACH;
> -		}
> +	if (unlikely(!READ_ONCE(vsock->started_once)) &&
> +	    !data_race(vhost_vq_get_backend(&vsock->vqs[VSOCK_VQ_RX]))) {
> +		rcu_read_unlock();
> +		kfree_skb(skb);
> +		return -EHOSTUNREACH;
>  	}
>  
>  	if (virtio_vsock_skb_reply(skb))
> @@ -637,6 +631,9 @@ static int vhost_vsock_start(struct vhost_vsock *vsock)
>  	 */
>  	vhost_vq_work_queue(&vsock->vqs[VSOCK_VQ_RX], &vsock->send_pkt_work);
>  
> +	/* See vhost_transport_send_pkt(); never cleared. */
> +	WRITE_ONCE(vsock->started_once, true);
> +
>  	mutex_unlock(&vsock->dev.mutex);
>  	return 0;
>