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

[Eugenio Perez Martin]

On Sat, May 6, 2023 at 5:01 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> 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 the SVQ. If there is
> enough space, then QEMU combines some out descriptors and
> some in descriptors into one descriptor chain, and add 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() return 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() return a guest's element,
> and use vhost_svq_add_elemnt(), a wrapper to vhost_svq_add(), to
> add this element into SVQ. 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() return `svq-vring.num` elements or
> descriptor chains, more than `svq->vring.num` descriptors, due to
> guest memory fragmentation, and this cause wrapping in SVQ descriptor ring.
>

The 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.

> Therefore, this patch adds `num_free` field in VhostShadowVirtqueue
> structure, updates this field in vhost_svq_add() and
> vhost_svq_get_buf(), to record the number of free descriptors.
> Then we can avoid wrap in SVQ descriptor ring by refactoring
> vhost_svq_available_slots().
>
> Fixes: 100890f7ca ("vhost: Shadow virtqueue buffers forwarding")
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
>  hw/virtio/vhost-shadow-virtqueue.c | 9 ++++++++-
>  hw/virtio/vhost-shadow-virtqueue.h | 3 +++
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c 
> b/hw/virtio/vhost-shadow-virtqueue.c
> index 8361e70d1b..e1c6952b10 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,9 @@ int vhost_svq_add(VhostShadowVirtqueue *svq, const struct 
> iovec *out_sg,
>          return -EINVAL;
>      }
>
> +    /* Update the size of SVQ vring free descriptors */
> +    svq->num_free -= ndescs;
> +
>      svq->desc_state[qemu_head].elem = elem;
>      svq->desc_state[qemu_head].ndescs = ndescs;
>      vhost_svq_kick(svq);
> @@ -450,6 +453,9 @@ static VirtQueueElement 
> *vhost_svq_get_buf(VhostShadowVirtqueue *svq,
>      svq->desc_next[last_used_chain] = svq->free_head;
>      svq->free_head = used_elem.id;
>
> +    /* Update the size of SVQ vring free descriptors */

No need for this comment.

Apart from that,

Acked-by: Eugenio Pérez <eperezma@redhat.com>

> +    svq->num_free += num;
> +
>      *len = used_elem.len;
>      return g_steal_pointer(&svq->desc_state[used_elem.id].elem);
>  }
> @@ -659,6 +665,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);
> --
> 2.25.1
>