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

[liu ping fan]

On Fri, May 3, 2013 at 6:02 PM, Stefan Hajnoczi <address@hidden> wrote:
> 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.
>
Will fix,
>>      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.
>
Ok,
>> +                                  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().
>
Ok,
>> @@ -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.
>
Will fix
>>      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.
Will fix.

Thanks,
Pingfan