Re: [PATCH 02/22] vfio/pci: Do not unparent in instance_finalize()

[Peter Xu <peterx@redhat.com>]

On Thu, Sep 11, 2025 at 12:47:24PM +0900, Akihiko Odaki wrote:
> On 2025/09/11 5:41, Peter Xu wrote:
> > On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
> > > Children are automatically unparented so manually unparenting is
> > > unnecessary.
> > > 
> > > Worse, automatic unparenting happens before the insntance_finalize()
> > > callback of the parent gets called, so object_unparent() calls in
> > > the callback will refer to objects that are already unparented, which
> > > is semantically incorrect.
> > > 
> > > Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> > > ---
> > >   hw/vfio/pci.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
> > >           vfio_region_finalize(&bar->region);
> > >           if (bar->mr) {
> > >               assert(bar->size);
> > > -            object_unparent(OBJECT(bar->mr));
> > >               g_free(bar->mr);
> > >               bar->mr = NULL;
> > >           }
> > > @@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
> > >       if (vdev->vga) {
> > >           vfio_vga_quirk_finalize(vdev);
> > > -        for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
> > > -            object_unparent(OBJECT(&vdev->vga->region[i].mem));
> > > -        }
> > >           g_free(vdev->vga);
> > >       }
> > >   }
> > 
> > So the 2nd object_unparent() here should be no-op, seeing empty list of
> > properties (but shouldn't causing anything severe), is that correct?
> 
> No. The object is finalized with the first object_unparent() if there is no
> referrer other than the parent. The second object_unparent() will access the
> finalized, invalid object in that case.

Yes, it's logically wrong.  I was trying to understand the impact when it's
invoked.  In this specific case of above two changes, I believe both MR
structs are still available, so it does look fine, e.g. nothing would crash.

For example, I think it doesn't need to copy stable if there's no real
functional issue involved.

> 
> > 
> > I think it still makes some sense to theoretically allow object_unparent()
> > to happen, at least when it happens before owner's finalize().  IIUC that
> > was the intention of the doc, pairing the memory_region_init*() operation.
> 
> Perhaps so, but this patch is only about the case where object_unparent() is
> called in finalize().

You ignored my other comment.  That (using object_new() on MRs) was what I
was thinking might be better than a split model you discussed here, so
that's also a comment for patch 1 of your series.

Btw, this patch also didn't change all occurances of such for VFIO?
E.g. there's at least vfio_vga_quirk_finalize().  I didn't check the rest.

-- 
Peter Xu