[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] vfio-pci: unparent BAR subregions
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH] vfio-pci: unparent BAR subregions |
Date: |
Sat, 31 Jan 2015 08:10:29 -0700 |
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 <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));
> > }
> > }
> >
> >
> >
> >