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 <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Signed-off-by: Jason Wang <address@hidden>

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