Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

[Jason Wang <jasowang@redhat.com>]

On 2020/5/11 下午5:11, Dima Stepanov wrote:
> On Mon, May 11, 2020 at 11:05:58AM +0800, Jason Wang wrote:
>> On 2020/4/30 下午9:36, Dima Stepanov wrote:
>>> Since disconnect can happen at any time during initialization not all
>>> vring buffers (for instance used vring) can be intialized successfully.
>>> If the buffer was not initialized then vhost_memory_unmap call will lead
>>> to SIGSEGV. Add checks for the vring address value before calling unmap.
>>> Also add assert() in the vhost_memory_unmap() routine.
>>>
>>> Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
>>> ---
>>>   hw/virtio/vhost.c | 27 +++++++++++++++++++++------
>>>   1 file changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> index ddbdc53..3ee50c4 100644
>>> --- a/hw/virtio/vhost.c
>>> +++ b/hw/virtio/vhost.c
>>> @@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void *buffer,
>>>                                  hwaddr len, int is_write,
>>>                                  hwaddr access_len)
>>>   {
>>> +    assert(buffer);
>>> +
>>>       if (!vhost_dev_has_iommu(dev)) {
>>>           cpu_physical_memory_unmap(buffer, len, is_write, access_len);
>>>       }
>>> @@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
>>>                                                   vhost_vq_index);
>>>       }
>>> -    vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
>>> -                       1, virtio_queue_get_used_size(vdev, idx));
>>> -    vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
>>> -                       0, virtio_queue_get_avail_size(vdev, idx));
>>> -    vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
>>> -                       0, virtio_queue_get_desc_size(vdev, idx));
>>> +    /*
>>> +     * Since the vhost-user disconnect can happen during initialization
>>> +     * check if vring was initialized, before making unmap.
>>> +     */
>>> +    if (vq->used) {
>>> +        vhost_memory_unmap(dev, vq->used,
>>> +                           virtio_queue_get_used_size(vdev, idx),
>>> +                           1, virtio_queue_get_used_size(vdev, idx));
>>> +    }
>>> +    if (vq->avail) {
>>> +        vhost_memory_unmap(dev, vq->avail,
>>> +                           virtio_queue_get_avail_size(vdev, idx),
>>> +                           0, virtio_queue_get_avail_size(vdev, idx));
>>> +    }
>>> +    if (vq->desc) {
>>> +        vhost_memory_unmap(dev, vq->desc,
>>> +                           virtio_queue_get_desc_size(vdev, idx),
>>> +                           0, virtio_queue_get_desc_size(vdev, idx));
>>> +    }
>>
>> Any reason not checking hdev->started instead? vhost_dev_start() will set it
>> to true if virtqueues were correctly mapped.
>>
>> Thanks
> Well i see it a little bit different:
>   - vhost_dev_start() sets hdev->started to true before starting
>     virtqueues
>   - vhost_virtqueue_start() maps all the memory
> If we hit the vhost disconnect at the start of the
> vhost_virtqueue_start(), for instance for this call:
>    r = dev->vhost_ops->vhost_set_vring_base(dev, &state);
> Then we will call vhost_user_blk_disconnect:
>    vhost_user_blk_disconnect()->
>      vhost_user_blk_stop()->
>        vhost_dev_stop()->
>          vhost_virtqueue_stop()
> As a result we will come in this routine with the hdev->started still
> set to true, but if used/avail/desc fields still uninitialized and set
> to 0.


I may miss something, but consider both vhost_dev_start() and 
vhost_user_blk_disconnect() were serialized in main loop. Can this 
really happen?

Thanks


>
>>
>>>   }
>>>   static void vhost_eventfd_add(MemoryListener *listener,