Re: [Qemu-devel] [PATCH v2 5/6] Vring: use hostmem's RAM safe api

[Stefan Hajnoczi]

On Fri, May 03, 2013 at 10:45:21AM +0800, Liu Ping Fan wrote:
> @@ -109,11 +110,14 @@ bool vring_should_notify(VirtIODevice *vdev, Vring 
> *vring)
>  static int get_indirect(Vring *vring,
>                          struct iovec iov[], struct iovec *iov_end,
>                          unsigned int *out_num, unsigned int *in_num,
> -                        struct vring_desc *indirect)
> +                        struct vring_desc *indirect,
> +                        MemoryRegion ***mrs)
>  {
>      struct vring_desc desc;
>      unsigned int i = 0, count, found = 0;
> -
> +    MemoryRegion **cur = *mrs;
> +    int ret = 0;
> +    MemoryRegion *tmp;
>      /* Sanity check */
>      if (unlikely(indirect->len % sizeof(desc))) {
>          error_report("Invalid length in indirect descriptor: "
> @@ -132,19 +136,23 @@ static int get_indirect(Vring *vring,
>          return -EFAULT;
>      }
>  
> +    *mrs[0] = NULL;

The goto fail case is broken when we fail with cur > *mrs before calling
hostmem_lookup(..., cur, ...) since *cur will be undefined.

>      do {
>          struct vring_desc *desc_ptr;
>  
>          /* Translate indirect descriptor */
>          desc_ptr = hostmem_lookup(indirect->addr + found * sizeof(desc),
> -                                  sizeof(desc), NULL, false);
> +                                  sizeof(desc),
> +                                  &tmp,

Please use a more descriptive name, like desc_mr.  This function is
fairly involved so the variable names should be descriptive.

> +                                  false);
>          if (!desc_ptr) {
>              error_report("Failed to map indirect descriptor "
>                           "addr %#" PRIx64 " len %zu",
>                           (uint64_t)indirect->addr + found * sizeof(desc),
>                           sizeof(desc));
>              vring->broken = true;
> -            return -EFAULT;
> +            ret = -EFAULT;
> +            goto fail;
>          }
>          desc = *desc_ptr;
>  
> @@ -155,31 +163,40 @@ static int get_indirect(Vring *vring,
>              error_report("Loop detected: last one at %u "
>                           "indirect size %u", i, count);
>              vring->broken = true;
> -            return -EFAULT;
> +            memory_region_unref(tmp);
> +            ret = -EFAULT;
> +            goto fail;
>          }

No need to hold onto tmp and handle all these error cases.  After the
barrier() desc_ptr is no longer used because we've loaded the descriptor
into a local struct.  Please unref tmp right after barrier().

> @@ -209,15 +240,20 @@ static int get_indirect(Vring *vring,
>   * never a valid descriptor number) if none was found.  A negative code is
>   * returned on error.
>   *
> + * @mrs should be the same cnt as iov[]
> + *
>   * Stolen from linux/drivers/vhost/vhost.c.
>   */
>  int vring_pop(VirtIODevice *vdev, Vring *vring,
>                struct iovec iov[], struct iovec *iov_end,
> -              unsigned int *out_num, unsigned int *in_num)
> +              unsigned int *out_num, unsigned int *in_num,
> +              MemoryRegion **mrs)
>  {
>      struct vring_desc desc;
>      unsigned int i, head, found = 0, num = vring->vr.num;
>      uint16_t avail_idx, last_avail_idx;
> +    MemoryRegion **indirect, **cur = mrs;
> +    int ret = 0;
>  
>      /* If there was a fatal error then refuse operation */
>      if (vring->broken) {
> @@ -263,17 +299,20 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
>      *out_num = *in_num = 0;
>  
>      i = head;
> +    mrs[0] = NULL;

Same goto fail issue here when cur > *mrs before
hostmem_lookup(..., cur, ...) has been called.

>      do {
>          if (unlikely(i >= num)) {
>              error_report("Desc index is %u > %u, head = %u", i, num, head);
>              vring->broken = true;
> -            return -EFAULT;
> +            ret = -EFAULT;
> +            goto fail;
>          }
>          if (unlikely(++found > num)) {
>              error_report("Loop detected: last one at %u vq size %u head %u",
>                           i, num, head);
>              vring->broken = true;
> -            return -EFAULT;
> +            ret = -EFAULT;
> +            goto fail;
>          }
>          desc = vring->vr.desc[i];
>  
> @@ -281,10 +320,13 @@ int vring_pop(VirtIODevice *vdev, Vring *vring,
>          barrier();
>  
>          if (desc.flags & VRING_DESC_F_INDIRECT) {
> -            int ret = get_indirect(vring, iov, iov_end, out_num, in_num, 
> &desc);
> +            indirect = cur;
> +            int ret = get_indirect(vring, iov, iov_end, out_num, in_num, 
> &desc,
> +                        &indirect);
>              if (ret < 0) {
> -                return ret;
> +               goto fail;

Indentation.