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

[Hawkins Jiawei]

Hi Eugenio,
Thanks for reviewing.

On 2023/5/9 1:26, Eugenio Perez Martin wrote:
> 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.

I will add this in the commit message in v2 patch.

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

Thanks for your suggestion. I will remove the
comment in v2 patch, with this tag on.


>> +    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
>>
>