On Mon, 18 Dec 2017 20:13:34 +0000
"Dr. David Alan Gilbert (git)" <dgilb...@redhat.com> wrote:
> From: "Dr. David Alan Gilbert" <dgilb...@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 <dgilb...@redhat.com>
As Michael've suggested it should go into stable (CCed)
Reviewed-by: Igor Mammedov <imamm...@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 */
