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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Igor Mammedov (imammedo@redhat.com) wrote:
> 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)

OK,

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

Thanks!

Dave

> 
> > ---
> >  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 */
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK