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

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

On Wed, Sep 17, 2025 at 02:17:35PM +0100, Daniel P. Berrangé wrote:
> 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.

Oh actually not a use-after-free since memory regions aren't directly
freed by object_finalize.


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 :|