Re: [PATCH v3 1/2] vhost: destroy device when all vqs are inactive

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 9/12/22 05:36, Kangjie Xu wrote:
> We change the behavior of vhost_user_get_vring_base(). Previosly,
> destroying a virtqueue will cause the whole device to be destroyed.
> The behavior is not specified in the vhost-user protocol.
> 
> Thus, we refactor this part. The device will be destroyed only when
> all virtqueues in the device are going to be destroyed.
> 
> This helps us to simplify the implementation when resetting a virtqueue.
> 
> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   lib/vhost/vhost_user.c | 10 ++++++++--
>   1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index 4ad28bac45..a9f0709f94 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -2088,10 +2088,16 @@ vhost_user_get_vring_base(struct virtio_net **pdev,
>   {
>   	struct virtio_net *dev = *pdev;
>   	struct vhost_virtqueue *vq = dev->virtqueue[ctx->msg.payload.state.index];
> +	uint32_t i, num_live_vring = 0;
>   	uint64_t val;
>   
> -	/* We have to stop the queue (virtio) if it is running. */
> -	vhost_destroy_device_notify(dev);
> +	/* Stop the device when vq is the last active queue */
> +	for (i = 0; i < dev->nr_vring; i++)
> +		if (dev->virtqueue[i]->access_ok)
> +			num_live_vring++;
> +
> +	if (num_live_vring == 1 && vq->access_ok)
> +		vhost_destroy_device_notify(dev);
>   
>   	dev->flags &= ~VIRTIO_DEV_READY;
>   	dev->flags &= ~VIRTIO_DEV_VDPA_CONFIGURED;

I think we are missing something here.

We used to send the device destroy notification before getting the ring
indexes, in order to ensure that the application has stopped processing
the rings.

With this patch, the application may still be polling the ring while we
get the ring indexes (e.g. a thread in the application may be in the
middle of rte_vhost_dequeue_burst() on that ring). So at best the ring
indexes returned to the Vhost-user master will be outdated. At worst, it
will crash the application because we call vring_invalidate() without
the vq's lock being taken.

I think you should protect all the VQ indexes fetching and VQ deinit
using its access_lock.

Maxime