Re: [Qemu-devel] [PATCH v5 1/7] vhost: Build temporary section list and deref after commit

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 18 Dec 2017 20:13:34 +0000
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Igor spotted that there's a race, where a region that's unref'd
> in a _del callback might be free'd before the set_mem_table call in
> the _commit callback, and thus the vhost might end up using free memory.
> 
> Fix this by building a complete temporary sections list, ref'ing every
> section (during add and nop) and then unref'ing the whole list right
> at the end of commit.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

As Michael've suggested it should go into stable (CCed)

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/virtio/vhost.c         | 73 ++++++++++++++++++++++++++++++-----------------
>  include/hw/virtio/vhost.h |  2 ++
>  2 files changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index e4290ce93d..610bfe6945 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -621,6 +621,8 @@ static void vhost_begin(MemoryListener *listener)
>                                           memory_listener);
>      dev->mem_changed_end_addr = 0;
>      dev->mem_changed_start_addr = -1;
> +    dev->tmp_sections = NULL;
> +    dev->n_tmp_sections = 0;
>  }
>  
>  static void vhost_commit(MemoryListener *listener)
> @@ -629,17 +631,25 @@ static void vhost_commit(MemoryListener *listener)
>                                           memory_listener);
>      hwaddr start_addr = 0;
>      ram_addr_t size = 0;
> +    MemoryRegionSection *old_sections;
> +    int n_old_sections;
> +
>      uint64_t log_size;
>      int r;
>  
> +    old_sections = dev->mem_sections;
> +    n_old_sections = dev->n_mem_sections;
> +    dev->mem_sections = dev->tmp_sections;
> +    dev->n_mem_sections = dev->n_tmp_sections;
> +
>      if (!dev->memory_changed) {
> -        return;
> +        goto out;
>      }
>      if (!dev->started) {
> -        return;
> +        goto out;
>      }
>      if (dev->mem_changed_start_addr > dev->mem_changed_end_addr) {
> -        return;
> +        goto out;
>      }
>  
>      if (dev->started) {
> @@ -656,7 +666,7 @@ static void vhost_commit(MemoryListener *listener)
>              VHOST_OPS_DEBUG("vhost_set_mem_table failed");
>          }
>          dev->memory_changed = false;
> -        return;
> +        goto out;
>      }
>      log_size = vhost_get_log_size(dev);
>      /* We allocate an extra 4K bytes to log,
> @@ -675,6 +685,27 @@ static void vhost_commit(MemoryListener *listener)
>          vhost_dev_log_resize(dev, log_size);
>      }
>      dev->memory_changed = false;
> +
> +out:
> +    /* Deref the old list of sections, this must happen _after_ the
> +     * vhost_set_mem_table to ensure the client isn't still using the
> +     * section we're about to unref.
> +     */
> +    while (n_old_sections--) {
> +        memory_region_unref(old_sections[n_old_sections].mr);
> +    }
> +    g_free(old_sections);
> +    return;
> +}
> +
> +static void vhost_add_section(struct vhost_dev *dev,
> +                              MemoryRegionSection *section)
> +{
> +    ++dev->n_tmp_sections;
> +    dev->tmp_sections = g_renew(MemoryRegionSection, dev->tmp_sections,
> +                                dev->n_tmp_sections);
> +    dev->tmp_sections[dev->n_tmp_sections - 1] = *section;
> +    memory_region_ref(section->mr);
>  }
>  
>  static void vhost_region_add(MemoryListener *listener,
> @@ -687,36 +718,31 @@ static void vhost_region_add(MemoryListener *listener,
>          return;
>      }
>  
> -    ++dev->n_mem_sections;
> -    dev->mem_sections = g_renew(MemoryRegionSection, dev->mem_sections,
> -                                dev->n_mem_sections);
> -    dev->mem_sections[dev->n_mem_sections - 1] = *section;
> -    memory_region_ref(section->mr);
> +    vhost_add_section(dev, section);
>      vhost_set_memory(listener, section, true);
>  }
>  
> -static void vhost_region_del(MemoryListener *listener,
> +static void vhost_region_nop(MemoryListener *listener,
>                               MemoryRegionSection *section)
>  {
>      struct vhost_dev *dev = container_of(listener, struct vhost_dev,
>                                           memory_listener);
> -    int i;
>  
>      if (!vhost_section(section)) {
>          return;
>      }
>  
> -    vhost_set_memory(listener, section, false);
> -    memory_region_unref(section->mr);
> -    for (i = 0; i < dev->n_mem_sections; ++i) {
> -        if (dev->mem_sections[i].offset_within_address_space
> -            == section->offset_within_address_space) {
> -            --dev->n_mem_sections;
> -            memmove(&dev->mem_sections[i], &dev->mem_sections[i+1],
> -                    (dev->n_mem_sections - i) * sizeof(*dev->mem_sections));
> -            break;
> -        }
> +    vhost_add_section(dev, section);
> +}
> +
> +static void vhost_region_del(MemoryListener *listener,
> +                             MemoryRegionSection *section)
> +{
> +    if (!vhost_section(section)) {
> +        return;
>      }
> +
> +    vhost_set_memory(listener, section, false);
>  }
>  
>  static void vhost_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
> @@ -783,11 +809,6 @@ static void vhost_iommu_region_del(MemoryListener *listener,
>      }
>  }
>  
> -static void vhost_region_nop(MemoryListener *listener,
> -                             MemoryRegionSection *section)
> -{
> -}
> -
>  static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
>                                      struct vhost_virtqueue *vq,
>                                      unsigned idx, bool enable_log)
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 467dc7794b..21f3022499 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -54,6 +54,8 @@ struct vhost_dev {
>      struct vhost_memory *mem;
>      int n_mem_sections;
>      MemoryRegionSection *mem_sections;
> +    int n_tmp_sections;
> +    MemoryRegionSection *tmp_sections;
>      struct vhost_virtqueue *vqs;
>      int nvqs;
>      /* the first virtqueue which would be used by this vhost dev */