[Qemu-devel] [PATCH] exec: don't return int64_t in address_space_cache_init()

[Qemu-devel] [PATCH] exec: don't return int64_t in address_space_cache_init()

[Jason Wang]

We return int64_t as the length of region cache but accept hwaddr as
the required length. This is wrong and may confuse the caller since
the it can lead comparison between signed and unsigned types. The
caller can not catch the failure in this case. Fixing this by
returning hwaddr and return zero on failure.

Fixes: 5eba0404b9829 ("virtio: use MemoryRegionCache to access descriptors")
Fixes: e45da65322386 ("virtio: validate address space cache during init")
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 exec.c                | 12 ++++++------
 hw/virtio/virtio.c    |  7 +++----
 include/exec/memory.h | 13 ++++++-------
 3 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/exec.c b/exec.c
index e57a8a2..9b71174 100644
--- a/exec.c
+++ b/exec.c
@@ -3230,11 +3230,11 @@ void cpu_physical_memory_unmap(void *buffer, hwaddr len,
 #define RCU_READ_UNLOCK(...)     rcu_read_unlock()
 #include "memory_ldst.inc.c"
 
-int64_t address_space_cache_init(MemoryRegionCache *cache,
-                                 AddressSpace *as,
-                                 hwaddr addr,
-                                 hwaddr len,
-                                 bool is_write)
+hwaddr address_space_cache_init(MemoryRegionCache *cache,
+                                AddressSpace *as,
+                                hwaddr addr,
+                                hwaddr len,
+                                bool is_write)
 {
     hwaddr l, xlat;
     MemoryRegion *mr;
@@ -3245,7 +3245,7 @@ int64_t address_space_cache_init(MemoryRegionCache *cache,
     l = len;
     mr = address_space_translate(as, addr, &xlat, &l, is_write);
     if (!memory_access_is_direct(mr, is_write)) {
-        return -EINVAL;
+        return 0;
     }
 
     l = address_space_extend_translation(as, addr, len, mr, xlat, l, is_write);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 03592c5..3482be2 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -129,9 +129,8 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
     VirtQueue *vq = &vdev->vq[n];
     VRingMemoryRegionCaches *old = vq->vring.caches;
     VRingMemoryRegionCaches *new;
-    hwaddr addr, size;
+    hwaddr addr, size, len;
     int event_size;
-    int64_t len;
 
     event_size = virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
 
@@ -586,7 +585,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
     unsigned int total_bufs, in_total, out_total;
     VRingMemoryRegionCaches *caches;
     MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
-    int64_t len = 0;
+    hwaddr len = 0;
     int rc;
 
     if (unlikely(!vq->vring.desc)) {
@@ -831,7 +830,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
     VRingMemoryRegionCaches *caches;
     MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
     MemoryRegionCache *desc_cache;
-    int64_t len;
+    hwaddr len;
     VirtIODevice *vdev = vq->vdev;
     VirtQueueElement *elem = NULL;
     unsigned out_num, in_num;
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e39256a..932dd00 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1444,8 +1444,7 @@ struct MemoryRegionCache {
  * @is_write: indicates the transfer direction
  *
  * Will only work with RAM, and may map a subset of the requested range by
- * returning a value that is less than @len.  On failure, return a negative
- * errno value.
+ * returning a value that is less than @len.  On failure, return zero.
  *
  * Because it only works with RAM, this function can be used for
  * read-modify-write operations.  In this case, is_write should be %true.
@@ -1453,11 +1452,11 @@ struct MemoryRegionCache {
  * Note that addresses passed to the address_space_*_cached functions
  * are relative to @addr.
  */
-int64_t address_space_cache_init(MemoryRegionCache *cache,
-                                 AddressSpace *as,
-                                 hwaddr addr,
-                                 hwaddr len,
-                                 bool is_write);
+hwaddr address_space_cache_init(MemoryRegionCache *cache,
+                                AddressSpace *as,
+                                hwaddr addr,
+                                hwaddr len,
+                                bool is_write);
 
 /**
  * address_space_cache_invalidate: complete a write to a #MemoryRegionCache
-- 
2.7.4

Re: [Qemu-devel] [PATCH] exec: don't return int64_t in address_space_cache_init()

[Michael S. Tsirkin]

On Wed, Mar 29, 2017 at 02:12:50PM +0800, Jason Wang wrote:
> We return int64_t as the length of region cache but accept hwaddr as
> the required length. This is wrong and may confuse the caller since
> the it can lead comparison between signed and unsigned types. The
> caller can not catch the failure in this case. Fixing this by
> returning hwaddr and return zero on failure.
> 
> Fixes: 5eba0404b9829 ("virtio: use MemoryRegionCache to access descriptors")
> Fixes: e45da65322386 ("virtio: validate address space cache during init")
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Can you be more specific about the symptoms this fixes in the
commit log?
E.g. "This actually triggers on XYZ when using ABC".


> ---
>  exec.c                | 12 ++++++------
>  hw/virtio/virtio.c    |  7 +++----
>  include/exec/memory.h | 13 ++++++-------
>  3 files changed, 15 insertions(+), 17 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index e57a8a2..9b71174 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3230,11 +3230,11 @@ void cpu_physical_memory_unmap(void *buffer, hwaddr len,
>  #define RCU_READ_UNLOCK(...)     rcu_read_unlock()
>  #include "memory_ldst.inc.c"
>  
> -int64_t address_space_cache_init(MemoryRegionCache *cache,
> -                                 AddressSpace *as,
> -                                 hwaddr addr,
> -                                 hwaddr len,
> -                                 bool is_write)
> +hwaddr address_space_cache_init(MemoryRegionCache *cache,
> +                                AddressSpace *as,
> +                                hwaddr addr,
> +                                hwaddr len,
> +                                bool is_write)
>  {
>      hwaddr l, xlat;
>      MemoryRegion *mr;
> @@ -3245,7 +3245,7 @@ int64_t address_space_cache_init(MemoryRegionCache *cache,
>      l = len;
>      mr = address_space_translate(as, addr, &xlat, &l, is_write);
>      if (!memory_access_is_direct(mr, is_write)) {
> -        return -EINVAL;
> +        return 0;
>      }
>  
>      l = address_space_extend_translation(as, addr, len, mr, xlat, l, is_write);
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 03592c5..3482be2 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -129,9 +129,8 @@ static void virtio_init_region_cache(VirtIODevice *vdev, int n)
>      VirtQueue *vq = &vdev->vq[n];
>      VRingMemoryRegionCaches *old = vq->vring.caches;
>      VRingMemoryRegionCaches *new;
> -    hwaddr addr, size;
> +    hwaddr addr, size, len;
>      int event_size;
> -    int64_t len;
>  
>      event_size = virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX) ? 2 : 0;
>  
> @@ -586,7 +585,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>      unsigned int total_bufs, in_total, out_total;
>      VRingMemoryRegionCaches *caches;
>      MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
> -    int64_t len = 0;
> +    hwaddr len = 0;
>      int rc;
>  
>      if (unlikely(!vq->vring.desc)) {
> @@ -831,7 +830,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
>      VRingMemoryRegionCaches *caches;
>      MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
>      MemoryRegionCache *desc_cache;
> -    int64_t len;
> +    hwaddr len;
>      VirtIODevice *vdev = vq->vdev;
>      VirtQueueElement *elem = NULL;
>      unsigned out_num, in_num;
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e39256a..932dd00 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1444,8 +1444,7 @@ struct MemoryRegionCache {
>   * @is_write: indicates the transfer direction
>   *
>   * Will only work with RAM, and may map a subset of the requested range by
> - * returning a value that is less than @len.  On failure, return a negative
> - * errno value.
> + * returning a value that is less than @len.  On failure, return zero.
>   *
>   * Because it only works with RAM, this function can be used for
>   * read-modify-write operations.  In this case, is_write should be %true.
> @@ -1453,11 +1452,11 @@ struct MemoryRegionCache {
>   * Note that addresses passed to the address_space_*_cached functions
>   * are relative to @addr.
>   */
> -int64_t address_space_cache_init(MemoryRegionCache *cache,
> -                                 AddressSpace *as,
> -                                 hwaddr addr,
> -                                 hwaddr len,
> -                                 bool is_write);
> +hwaddr address_space_cache_init(MemoryRegionCache *cache,
> +                                AddressSpace *as,
> +                                hwaddr addr,
> +                                hwaddr len,
> +                                bool is_write);
>  
>  /**
>   * address_space_cache_invalidate: complete a write to a #MemoryRegionCache
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH] exec: don't return int64_t in address_space_cache_init()

[Jason Wang]

On 2017年03月30日 05:26, Michael S. Tsirkin wrote:
> On Wed, Mar 29, 2017 at 02:12:50PM +0800, Jason Wang wrote:
>> We return int64_t as the length of region cache but accept hwaddr as
>> the required length. This is wrong and may confuse the caller since
>> the it can lead comparison between signed and unsigned types. The
>> caller can not catch the failure in this case. Fixing this by
>> returning hwaddr and return zero on failure.
>>
>> Fixes: 5eba0404b9829 ("virtio: use MemoryRegionCache to access descriptors")
>> Fixes: e45da65322386 ("virtio: validate address space cache during init")
>> Cc: Cornelia Huck<cornelia.huck@de.ibm.com>
>> Cc: Paolo Bonzini<pbonzini@redhat.com>
>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> Can you be more specific about the symptoms this fixes in the
> commit log?
> E.g. "This actually triggers on XYZ when using ABC".
>
>

I want do this, but in fact this was triggered by a bug of qemu (see the 
thread of iommu reset vs region cache).

In that case, when used map fails, then check

      if (len < size) {
         virtio_error(vdev, "Cannot map used");
         goto err_used;
     }

can not catch the -EFAULT, since len is converted to unsigned.

Thanks