Re: [Qemu-devel] [PATCH] vfio-pci: unparent BAR subregions

[Alex Williamson <alex.williamson@redhat.com>]

On Sat, 2015-01-31 at 09:43 +0100, Paolo Bonzini wrote:
> 
> On 31/01/2015 00:55, Alex Williamson wrote:
> > Commit d8d95814609e replaced a number of memory_region_destroy()
> > calls with object_unparent() calls.  The logic appears to be that
> > subregions need to be unparented, but the base region is destroyed
> > with the device object.  Doing hotplug testing with vfio-pci I
> > occasionally get a segfault from object_finalize_child_property()
> > due to completely bogus class pointers on the child Object.  Adding
> > the explicit object_unparent() for these subregions resolves the
> > problem, however I question the sanity of the Memory API now where
> > we sometimes need to destroy MemoryRegions, but the rules aren't
> > clear
> 
> There is no memory_region_destroy API because you cannot destroy
> MemoryRegions.  All you do is releasing the link between the VFIO device
> (the parent, specified in memory_region_init*) and the MemoryRegion.
> The link caused the VFIO device to keep the MemoryRegion alive.
> 
> There can be pending references to the VFIO device at unrealize time,
> and this is why the memory_region_destroy() API was not enough.  For
> example if someone was doing I/O to a BAR and thus address_space_map is
> keeping the VFIO device alive.
> 
> The explicit memory_region_destroy() function made it much harder to
> handle this case.  You had to define an instance_finalize function for
> every class, and do memory_region_destroy() there.  Not surprisingly, no
> one did that.  Sure, it's not a common case and a well-behaving guest
> does not do that, but if it does it means use-after-frees and thus a
> possible guest->host escalation.
> 
> Instead, the implicit destruction via reference counting makes this case
> easy to handle, because reclamation is done automatically when the VFIO
> device dies.
> 
> Explicit object_unparent() is only needed if you recreate the memory
> region during the lifetime of the object.  This is rarely needed, and it
> is simple to spot if it's needed.  If you do memory_region_init* outside
> the realize function, most likely you need a matching object_unparent
> somewhere else in the device logic.
> 
> This was the idea behind commit d8d95814609e.  It only touched a handful
> of files because almost no one does memory_region_init* outside the
> realize function, and in particular VFIO doesn't.  VFIO follows the
> common convention of only creating regions in realize, and thus does not
> need object_unparent.
> 
> > and there's no longer a memory_region_destroy() function, so
> > we need to reach over to some other random QEMU API
> 
> It's not random.  Object is the parent class of MemoryRegion.
> object_unparent is a method for MemoryRegion.
> 
> > and unparent an object that we barely know about
> 
> I'm not sure about this?  You certainly know the memory regions you create.
> 
> > and certainly didn't explicitly parent previously.
> 
> You did when you passed the VFIO device to memory_region_init*.
> 
> I'm afraid this patch is incorrect.  You have to find out where the
> region is being overwritten.

Thanks Paolo, so if I look more closely at where you added
object_unparent() calls in d8d95814609e, I can see that they're
associated with dynamically allocated objects that are freed as part of
the vfio device exitfn.  vdev->msix is also such a structure and is the
property causing us the segfaults.  Being associated with a free also
explains the randomness of the segfault.  So, I think the second
object_unparent() call is correct and that the guiding principle is that
any MemoryRegion associated with a dynamically allocated structure and
freed as part of the class exit callback needs to be explicitly
unparented.  Does that sound right?  Thanks,

Alex

> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: qemu-stable@nongnu.org
> > ---
> > 
> >  hw/vfio/pci.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > index 014a92c..c71499e 100644
> > --- a/hw/vfio/pci.c
> > +++ b/hw/vfio/pci.c
> > @@ -2294,10 +2294,12 @@ static void vfio_unmap_bar(VFIOPCIDevice *vdev, int nr)
> >  
> >      memory_region_del_subregion(&bar->region.mem, &bar->region.mmap_mem);
> >      munmap(bar->region.mmap, memory_region_size(&bar->region.mmap_mem));
> > +    object_unparent(OBJECT(&bar->region.mmap_mem));
> >  
> >      if (vdev->msix && vdev->msix->table_bar == nr) {
> >          memory_region_del_subregion(&bar->region.mem, &vdev->msix->mmap_mem);
> >          munmap(vdev->msix->mmap, memory_region_size(&vdev->msix->mmap_mem));
> > +        object_unparent(OBJECT(&vdev->msix->mmap_mem));
> >      }
> >  }
> >  
> > 
> > 
> >