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

[Paolo Bonzini]

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

> Signed-off-by: Alex Williamson <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Cc: address@hidden
> ---
> 
>  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));
>      }
>  }
>  
> 
> 
>