Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Daniel P. Berrangé <berrange@xxxxxxxxxx>]

On Wed, Sep 17, 2025 at 09:24:04PM +0900, Akihiko Odaki wrote:
> On 2025/09/17 20:57, Daniel P. Berrangé wrote:
> > On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> > > Based-on: <cover.1751493467.git.balaton@xxxxxxxxxx>
> > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > > 
> > > Supersedes: <20240829-memory-v1-1-ac07af2f4fa5@xxxxxxxxxx>
> > > ("[PATCH] docs/devel: Prohibit calling object_unparent() for memory 
> > > region")
> > > 
> > > Children are automatically unparented so manually unparenting is
> > > unnecessary.
> > 
> > Where is automatic unparenting you're referring to being done ?
> > 
> > > Worse, automatic unparenting happens before the instance_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.
> > 
> > IIUC, object_property_add_child will acquire a reference on
> > the child, and object_property_del_child (and thus
> > object_unparent) will release that reference.
> > 
> > The 'object_finalize' method, and thus 'instance_finalize'
> > callback, won't be invoked until the last reference is
> > dropped on the object in question.
> > 
> > IOW, it should be impossible for 'object_finalize' to ever
> > run, as long as the child has a parent set.
> > 
> > So if we're in the 'finalize' then 'object_unparent' must
> > be a no-op as the child must already have no references
> > held and thus no parent.
> > 
> > IOW, the reason to remove 'object_unparent' calls from
> > finalize is surely because they do nothing at all,
> > rather than this talk about callbacks being run at the
> > wrong time ?
> 
> This patch series deals with the situation where the parent calls
> object_unparent() in its instance_finalize() callback. The process of
> finalization looks like as follows:
> 
> 1. The parent's reference count reaches to zero. Please note that there can
> be remaining children that are referenced by the parent at this point.
> 
> 2. object_finalize() is called.
> 
> 2a. object_property_del_all() is called and the parent releases references
> to its children. This is what I referred as "automatic unparenting". The
> children without any other references will be finalized here.
> 
> 2b. instance_finalize() is called. Past children may be already finalized,
> and calling object_unparent() here will cause dereferencing finalized
> objects in that case, which should be avoided.

Oh, so these object_unparent calls run by the parent, against the child
in fact use-after-free flaws.

This is driven by the parent keeping hold of explicit pointers to the
child (MemoryRegion), without also holding its own reference, and these
pointers are invalidated when the parent<->child property is deleted.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|