[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 4/9] virtio: handle virtqueue_map_desc() erro
From: |
Greg Kurz |
Subject: |
Re: [Qemu-devel] [PATCH v4 4/9] virtio: handle virtqueue_map_desc() errors |
Date: |
Wed, 21 Sep 2016 09:02:35 +0200 |
On Tue, 20 Sep 2016 15:49:33 +0100
Stefan Hajnoczi <address@hidden> wrote:
> Errors can occur during virtqueue_pop(), especially in
> virtqueue_map_desc(). In order to handle this we must unmap iov[]
> before returning NULL. The caller will consider the virtqueue empty and
> the virtio_error() call will have marked the device broken.
>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
Hi Stefan,
FWIW, Prasad's "virtio: add check for descriptor's mapped address" is already
in Michael's tree:
https://git.kernel.org/cgit/virt/kvm/mst/qemu.git/commit/?h=pci&id=13c9ed60de6faaed325804620d13e7be37ea8183
I guess this patch should take it into account (as already suggested by Laszlo).
Cheers.
--
Greg
> hw/virtio/virtio.c | 70
> +++++++++++++++++++++++++++++++++++++++++-------------
> 1 file changed, 53 insertions(+), 17 deletions(-)
>
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index a841640..c499028 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -484,24 +484,27 @@ int virtqueue_avail_bytes(VirtQueue *vq, unsigned int
> in_bytes,
> return in_bytes <= in_total && out_bytes <= out_total;
> }
>
> -static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct
> iovec *iov,
> +static bool virtqueue_map_desc(VirtIODevice *vdev, unsigned int *p_num_sg,
> + hwaddr *addr, struct iovec *iov,
> unsigned int max_num_sg, bool is_write,
> hwaddr pa, size_t sz)
> {
> + bool ok = false;
> unsigned num_sg = *p_num_sg;
> assert(num_sg <= max_num_sg);
>
> if (!sz) {
> - error_report("virtio: zero sized buffers are not allowed");
> - exit(1);
> + virtio_error(vdev, "virtio: zero sized buffers are not allowed");
> + goto out;
> }
>
> while (sz) {
> hwaddr len = sz;
>
> if (num_sg == max_num_sg) {
> - error_report("virtio: too many write descriptors in indirect
> table");
> - exit(1);
> + virtio_error(vdev, "virtio: too many write descriptors in "
> + "indirect table");
> + goto out;
> }
>
> iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write);
> @@ -512,7 +515,28 @@ static void virtqueue_map_desc(unsigned int *p_num_sg,
> hwaddr *addr, struct iove
> pa += len;
> num_sg++;
> }
> + ok = true;
> +
> +out:
> *p_num_sg = num_sg;
> + return ok;
> +}
> +
> +/* Only used by error code paths before we have a VirtQueueElement (therefore
> + * virtqueue_unmap_sg() can't be used). Assumes buffers weren't written to
> + * yet.
> + */
> +static void virtqueue_undo_map_desc(unsigned out_num, unsigned in_num,
> + struct iovec *iov)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < out_num + in_num; i++) {
> + int is_write = i >= out_num;
> +
> + cpu_physical_memory_unmap(iov->iov_base, iov->iov_len, is_write, 0);
> + iov++;
> + }
> }
>
> static void virtqueue_map_iovec(struct iovec *sg, hwaddr *addr,
> @@ -604,8 +628,8 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
> max = vq->vring.num;
>
> if (vq->inuse >= vq->vring.num) {
> - error_report("Virtqueue size exceeded");
> - exit(1);
> + virtio_error(vdev, "Virtqueue size exceeded");
> + return NULL;
> }
>
> i = head = virtqueue_get_head(vq, vq->last_avail_idx++);
> @@ -616,8 +640,8 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
> vring_desc_read(vdev, &desc, desc_pa, i);
> if (desc.flags & VRING_DESC_F_INDIRECT) {
> if (desc.len % sizeof(VRingDesc)) {
> - error_report("Invalid size for indirect buffer table");
> - exit(1);
> + virtio_error(vdev, "Invalid size for indirect buffer table");
> + return NULL;
> }
>
> /* loop over the indirect descriptor table */
> @@ -629,22 +653,30 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>
> /* Collect all the descriptors */
> do {
> + bool map_ok;
> +
> if (desc.flags & VRING_DESC_F_WRITE) {
> - virtqueue_map_desc(&in_num, addr + out_num, iov + out_num,
> - VIRTQUEUE_MAX_SIZE - out_num, true,
> desc.addr, desc.len);
> + map_ok = virtqueue_map_desc(vdev, &in_num, addr + out_num,
> + iov + out_num,
> + VIRTQUEUE_MAX_SIZE - out_num, true,
> + desc.addr, desc.len);
> } else {
> if (in_num) {
> - error_report("Incorrect order for descriptors");
> - exit(1);
> + virtio_error(vdev, "Incorrect order for descriptors");
> + goto err_undo_map;
> }
> - virtqueue_map_desc(&out_num, addr, iov,
> - VIRTQUEUE_MAX_SIZE, false, desc.addr,
> desc.len);
> + map_ok = virtqueue_map_desc(vdev, &out_num, addr, iov,
> + VIRTQUEUE_MAX_SIZE, false,
> + desc.addr, desc.len);
> + }
> + if (!map_ok) {
> + goto err_undo_map;
> }
>
> /* If we've got too many, that implies a descriptor loop. */
> if ((in_num + out_num) > max) {
> - error_report("Looped descriptor");
> - exit(1);
> + virtio_error(vdev, "Looped descriptor");
> + goto err_undo_map;
> }
> } while ((i = virtqueue_read_next_desc(vdev, &desc, desc_pa, max)) !=
> max);
>
> @@ -664,6 +696,10 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>
> trace_virtqueue_pop(vq, elem, elem->in_num, elem->out_num);
> return elem;
> +
> +err_undo_map:
> + virtqueue_undo_map_desc(out_num, in_num, iov);
> + return NULL;
> }
>
> /* Reading and writing a structure directly to QEMUFile is *awful*, but
- [Qemu-devel] [PATCH v4 0/9] virtio: avoid exit() when device enters invalid states, Stefan Hajnoczi, 2016/09/20
- [Qemu-devel] [PATCH v4 1/9] virtio: fix stray tab character, Stefan Hajnoczi, 2016/09/20
- [Qemu-devel] [PATCH v4 2/9] virtio: stop virtqueue processing if device is broken, Stefan Hajnoczi, 2016/09/20
- [Qemu-devel] [PATCH v4 3/9] virtio: migrate vdev->broken flag, Stefan Hajnoczi, 2016/09/20
- [Qemu-devel] [PATCH v4 4/9] virtio: handle virtqueue_map_desc() errors, Stefan Hajnoczi, 2016/09/20
- Re: [Qemu-devel] [PATCH v4 4/9] virtio: handle virtqueue_map_desc() errors,
Greg Kurz <=
- [Qemu-devel] [PATCH v4 6/9] virtio: use unsigned int for virtqueue_get_avail_bytes() index, Stefan Hajnoczi, 2016/09/20
- [Qemu-devel] [PATCH v4 7/9] virtio: handle virtqueue_read_next_desc() errors, Stefan Hajnoczi, 2016/09/20
- [Qemu-devel] [PATCH v4 5/9] virtio: handle virtqueue_get_avail_bytes() errors, Stefan Hajnoczi, 2016/09/20
- [Qemu-devel] [PATCH v4 8/9] virtio: handle virtqueue_num_heads() errors, Stefan Hajnoczi, 2016/09/20
- [Qemu-devel] [PATCH v4 9/9] virtio: handle virtqueue_get_head() errors, Stefan Hajnoczi, 2016/09/20
- Re: [Qemu-devel] [PATCH v4 0/9] virtio: avoid exit() when device enters invalid states, Cornelia Huck, 2016/09/21