On Wed, Sep 17, 2025 at 02:23:35PM +0100, Daniel P. Berrangé wrote:
> 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.bala...@eik.bme.hu>
> > > > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > > > >
> > > > > Supersedes: <20240829-memory-v1-1-ac07af2f4...@daynix.com>
> > > > > ("[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.
We discussed this previously, I think so far it's 100% safe to call
object_unparent() twice, because step (2a) will reset child->parent=NULL.
Then at (2b) calling object_unparent() will be 100% safe because it's no-op
for an object that is orphaned.
So the series looks good, but it's kind of a cleanup, as object_unparent()
is just unnecessary for these MRs, same to the memory.rst doc suggestions
which can be avoided.
Thanks,
--
Peter Xu
