Re: [PATCH v3 2/5] virtio: Track shared memory mappings

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 9801 bytes --]

On Thu, Sep 12, 2024 at 04:53:32PM +0200, Albert Esteve wrote:
> Update shmem_list to be able to track
> active mappings on VIRTIO shared memory
> regions. This allows to verify that new
> mapping request received from backends
> do not overlap. If they do, the request
> shall fail in order to adhere to the specs.
> 
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>  hw/virtio/vhost-user.c     | 31 +++++++++++++-------
>  hw/virtio/virtio.c         | 58 ++++++++++++++++++++++++++++++++++----
>  include/hw/virtio/virtio.h | 25 ++++++++++++++--
>  3 files changed, 96 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 338cc942ec..de0bb35257 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1776,7 +1776,7 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
>                                      int fd)
>  {
>      void *addr = 0;
> -    MemoryRegion *mr = NULL;
> +    VirtSharedMemory *shmem = NULL;
>  
>      if (fd < 0) {
>          error_report("Bad fd for map");
> @@ -1791,22 +1791,29 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
>          return -EFAULT;
>      }
>  
> -    mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> +    shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
>  
> -    if (!mr) {
> +    if (!shmem) {
>          error_report("VIRTIO Shared Memory Region at "
>                       "ID %d unitialized", vu_mmap->shmid);
>          return -EFAULT;
>      }
>  
>      if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> -        (vu_mmap->shm_offset + vu_mmap->len) > mr->size) {
> +        (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
>          error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
>                       vu_mmap->shm_offset, vu_mmap->len);
>          return -EFAULT;
>      }
>  
> -    void *shmem_ptr = memory_region_get_ram_ptr(mr);
> +    if (virtio_shmem_map_overlaps(shmem, vu_mmap->shm_offset, vu_mmap->len)) {
> +        error_report("Requested memory (%" PRIx64 "+%" PRIx64 ") overalps "
> +                     "with previously mapped memory",
> +                     vu_mmap->shm_offset, vu_mmap->len);
> +        return -EFAULT;
> +    }
> +
> +    void *shmem_ptr = memory_region_get_ram_ptr(shmem->mr);
>  
>      addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
>          ((vu_mmap->flags & VHOST_USER_FLAG_MAP_R) ? PROT_READ : 0) |
> @@ -1818,6 +1825,8 @@ vhost_user_backend_handle_shmem_map(struct vhost_dev *dev,
>          return -EFAULT;
>      }
>  
> +    virtio_add_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> +
>      return 0;
>  }
>  
> @@ -1826,7 +1835,7 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
>                                        VhostUserMMap *vu_mmap)
>  {
>      void *addr = 0;
> -    MemoryRegion *mr = NULL;
> +    VirtSharedMemory *shmem = NULL;
>  
>      if (!dev->vdev->shmem_list ||
>          dev->vdev->n_shmem_regions <= vu_mmap->shmid) {
> @@ -1836,22 +1845,22 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
>          return -EFAULT;
>      }
>  
> -    mr = &dev->vdev->shmem_list[vu_mmap->shmid];
> +    shmem = &dev->vdev->shmem_list[vu_mmap->shmid];
>  
> -    if (!mr) {
> +    if (!shmem) {
>          error_report("VIRTIO Shared Memory Region at "
>                       "ID %d unitialized", vu_mmap->shmid);
>          return -EFAULT;
>      }
>  
>      if ((vu_mmap->shm_offset + vu_mmap->len) < vu_mmap->len ||
> -        (vu_mmap->shm_offset + vu_mmap->len) > mr->size) {
> +        (vu_mmap->shm_offset + vu_mmap->len) > shmem->mr->size) {
>          error_report("Bad offset/len for mmap %" PRIx64 "+%" PRIx64,
>                       vu_mmap->shm_offset, vu_mmap->len);
>          return -EFAULT;
>      }

Please add a check for an existing mapping that matches [shm_offset,
shm_offset + len). This will prevent partial unmap as required by the
spec.

>  
> -    void *shmem_ptr = memory_region_get_ram_ptr(mr);
> +    void *shmem_ptr = memory_region_get_ram_ptr(shmem->mr);
>  
>      addr = mmap(shmem_ptr + vu_mmap->shm_offset, vu_mmap->len,
>                  PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
> @@ -1861,6 +1870,8 @@ vhost_user_backend_handle_shmem_unmap(struct vhost_dev *dev,
>          return -EFAULT;
>      }
>  
> +    virtio_del_shmem_map(shmem, vu_mmap->shm_offset, vu_mmap->len);
> +
>      return 0;
>  }
>  
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index ccc4f2cd75..0e2cd62a15 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -3059,15 +3059,52 @@ int virtio_save(VirtIODevice *vdev, QEMUFile *f)
>      return vmstate_save_state(f, &vmstate_virtio, vdev, NULL);
>  }
>  
> -MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev)
> +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev)
>  {
> -    MemoryRegion *mr;
> +    VirtSharedMemory *shmem = NULL;
>      ++vdev->n_shmem_regions;
> -    vdev->shmem_list = g_renew(MemoryRegion, vdev->shmem_list,
> +    vdev->shmem_list = g_renew(VirtSharedMemory, vdev->shmem_list,
>                                 vdev->n_shmem_regions);
> -    mr = &vdev->shmem_list[vdev->n_shmem_regions - 1];
> -    mr = g_new0(MemoryRegion, 1);
> -    return mr;
> +    shmem = &vdev->shmem_list[vdev->n_shmem_regions - 1];
> +    shmem = g_new0(VirtSharedMemory, 1);
> +    QTAILQ_INIT(&shmem->mapped_regions);
> +    return shmem;
> +}
> +
> +void virtio_add_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> +                          uint64_t size)
> +{
> +    MappedMemoryRegion *mmap = g_new0(MappedMemoryRegion, 1);
> +    mmap->offset = offset;
> +    mmap->size = int128_make64(size);
> +    QTAILQ_REMOVE(&shmem->mapped_regions, mmap, link);
> +    g_free(mmap);

mmap needs to be inserted into mapped_regions and must not be freed by
this function.

> +}
> +
> +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> +                          uint64_t size)
> +{
> +    MappedMemoryRegion *mmap = g_new0(MappedMemoryRegion, 1);
> +    mmap->offset = offset;
> +    mmap->size = int128_make64(size);
> +    QTAILQ_INSERT_TAIL(&shmem->mapped_regions, mmap, link);

This function needs to search the list for the matching mmap and remove
it from the list. It should not create a new mmap.

> +    g_free(mmap);
> +}
> +
> +bool virtio_shmem_map_overlaps(VirtSharedMemory *shmem, hwaddr offset,
> +                               uint64_t size)
> +{
> +    MappedMemoryRegion *map_reg;
> +    hwaddr new_reg_end = offset + size;
> +    QTAILQ_FOREACH(map_reg, &shmem->mapped_regions, link) {
> +        hwaddr region_end = map_reg->offset + map_reg->size;
> +        if ((map_reg->offset == offset) ||
> +            (map_reg->offset < offset && region_end >= offset) ||
> +            (offset < map_reg->offset && new_reg_end >= map_reg->offset )) {
> +            return true;
> +        }
> +    }
> +    return false;   
>  }
>  
>  /* A wrapper for use as a VMState .put function */
> @@ -4007,11 +4044,20 @@ static void virtio_device_free_virtqueues(VirtIODevice *vdev)
>  static void virtio_device_instance_finalize(Object *obj)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(obj);
> +    VirtSharedMemory *shmem = NULL;
> +    int i;
>  
>      virtio_device_free_virtqueues(vdev);
>  
>      g_free(vdev->config);
>      g_free(vdev->vector_queues);
> +    for (i = 0; i< vdev->n_shmem_regions; i++) {
> +        shmem = &vdev->shmem_list[i];
> +        while (!QTAILQ_EMPTY(&shmem->mapped_regions)) {
> +            MappedMemoryRegion *mmap_reg = QTAILQ_FIRST(&shmem->mapped_regions);
> +            QTAILQ_REMOVE(&shmem->mapped_regions, mmap_reg, link);

g_free(mmap_reg)

> +        }
> +    }
>  }
>  
>  static Property virtio_properties[] = {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index d4a2f664d9..5b801f33f5 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -98,6 +98,21 @@ enum virtio_device_endian {
>      VIRTIO_DEVICE_ENDIAN_BIG,
>  };
>  
> +struct MappedMemoryRegion {
> +    Int128 size;
> +    hwaddr offset;
> +    QTAILQ_ENTRY(MappedMemoryRegion) link;
> +};
> +
> +typedef struct MappedMemoryRegion MappedMemoryRegion;
> +
> +struct VirtSharedMemory {
> +    MemoryRegion *mr;
> +    QTAILQ_HEAD(, MappedMemoryRegion) mapped_regions;
> +};
> +
> +typedef struct VirtSharedMemory VirtSharedMemory;
> +
>  /**
>   * struct VirtIODevice - common VirtIO structure
>   * @name: name of the device
> @@ -168,7 +183,7 @@ struct VirtIODevice
>      EventNotifier config_notifier;
>      bool device_iotlb_enabled;
>      /* Shared memory region for vhost-user mappings. */
> -    MemoryRegion *shmem_list;
> +    VirtSharedMemory *shmem_list;
>      int n_shmem_regions;
>  };
>  
> @@ -289,7 +304,13 @@ void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>  
>  int virtio_save(VirtIODevice *vdev, QEMUFile *f);
>  
> -MemoryRegion *virtio_new_shmem_region(VirtIODevice *vdev);
> +VirtSharedMemory *virtio_new_shmem_region(VirtIODevice *vdev);
> +void virtio_add_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> +                          uint64_t size);
> +void virtio_del_shmem_map(VirtSharedMemory *shmem, hwaddr offset,
> +                          uint64_t size);
> +bool virtio_shmem_map_overlaps(VirtSharedMemory *shmem, hwaddr offset,
> +                               uint64_t size);
>  
>  extern const VMStateInfo virtio_vmstate_info;
>  
> -- 
> 2.45.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]