Re: [PATCH v2] vhost: fix possible wrap in SVQ descriptor ring

[Jason Wang <jasowang@redhat.com>]

在 2023/5/9 16:48, Hawkins Jiawei 写道:
> QEMU invokes vhost_svq_add() when adding a guest's element
> into SVQ. In vhost_svq_add(), it uses vhost_svq_available_slots()
> to check whether QEMU can add the element into SVQ. If there is
> enough space, then QEMU combines some out descriptors and some
> in descriptors into one descriptor chain, and adds it into
> `svq->vring.desc` by vhost_svq_vring_write_descs().
>
> Yet the problem is that, `svq->shadow_avail_idx - svq->shadow_used_idx`
> in vhost_svq_available_slots() returns the number of occupied elements,
> or the number of descriptor chains, instead of the number of occupied
> descriptors, which may cause wrapping in SVQ descriptor ring.
>
> Here is an example. In vhost_handle_guest_kick(), QEMU forwards
> as many available buffers to device by virtqueue_pop() and
> vhost_svq_add_element(). virtqueue_pop() returns a guest's element,
> and then this element is added into SVQ by vhost_svq_add_element(),
> a wrapper to vhost_svq_add(). If QEMU invokes virtqueue_pop() and
> vhost_svq_add_element() `svq->vring.num` times,
> vhost_svq_available_slots() thinks QEMU just ran out of slots and
> everything should work fine. But in fact, virtqueue_pop() returns
> `svq->vring.num` elements or descriptor chains, more than
> `svq->vring.num` descriptors due to guest memory fragmentation,
> and this causes wrapping in SVQ descriptor ring.
>
> This bug is valid even before marking the descriptors used.
> If the guest memory is fragmented, SVQ must add chains
> so it can try to add more descriptors than possible.
>
> This patch solves it by adding `num_free` field in
> VhostShadowVirtqueue structure and updating this field
> in vhost_svq_add() and vhost_svq_get_buf(), to record
> the number of free descriptors.
>
> Fixes: 100890f7ca ("vhost: Shadow virtqueue buffers forwarding")
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>


Acked-by: Jason Wang <jasowang@redhat.com>

Thanks


> ---
> v2:
>    - update the commit message
>    - remove the unnecessary comment
>    - add the Acked-by tag
>
> v1: https://lists.gnu.org/archive/html/qemu-devel/2023-05/msg01727.html
>
>   hw/virtio/vhost-shadow-virtqueue.c | 5 ++++-
>   hw/virtio/vhost-shadow-virtqueue.h | 3 +++
>   2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 8361e70d1b..bd7c12b6d3 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -68,7 +68,7 @@ bool vhost_svq_valid_features(uint64_t features, Error **errp)
>    */
>   static uint16_t vhost_svq_available_slots(const VhostShadowVirtqueue *svq)
>   {
> -    return svq->vring.num - (svq->shadow_avail_idx - svq->shadow_used_idx);
> +    return svq->num_free;
>   }
>   
>   /**
> @@ -263,6 +263,7 @@ int vhost_svq_add(VhostShadowVirtqueue *svq, const struct iovec *out_sg,
>           return -EINVAL;
>       }
>   
> +    svq->num_free -= ndescs;
>       svq->desc_state[qemu_head].elem = elem;
>       svq->desc_state[qemu_head].ndescs = ndescs;
>       vhost_svq_kick(svq);
> @@ -449,6 +450,7 @@ static VirtQueueElement *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
>       last_used_chain = vhost_svq_last_desc_of_chain(svq, num, used_elem.id);
>       svq->desc_next[last_used_chain] = svq->free_head;
>       svq->free_head = used_elem.id;
> +    svq->num_free += num;
>   
>       *len = used_elem.len;
>       return g_steal_pointer(&svq->desc_state[used_elem.id].elem);
> @@ -659,6 +661,7 @@ void vhost_svq_start(VhostShadowVirtqueue *svq, VirtIODevice *vdev,
>       svq->iova_tree = iova_tree;
>   
>       svq->vring.num = virtio_queue_get_num(vdev, virtio_get_queue_index(vq));
> +    svq->num_free = svq->vring.num;
>       driver_size = vhost_svq_driver_area_size(svq);
>       device_size = vhost_svq_device_area_size(svq);
>       svq->vring.desc = qemu_memalign(qemu_real_host_page_size(), driver_size);
> diff --git a/hw/virtio/vhost-shadow-virtqueue.h b/hw/virtio/vhost-shadow-virtqueue.h
> index 926a4897b1..6efe051a70 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.h
> +++ b/hw/virtio/vhost-shadow-virtqueue.h
> @@ -107,6 +107,9 @@ typedef struct VhostShadowVirtqueue {
>   
>       /* Next head to consume from the device */
>       uint16_t last_used_idx;
> +
> +    /* Size of SVQ vring free descriptors */
> +    uint16_t num_free;
>   } VhostShadowVirtqueue;
>   
>   bool vhost_svq_valid_features(uint64_t features, Error **errp);