docs/devel/memory.rst | 19 ++++++------------- hw/core/register.c | 1 - hw/hyperv/hv-balloon.c | 12 +----------- hw/sd/sdhci.c | 4 ---- hw/vfio/pci-quirks.c | 9 +-------- hw/vfio/pci.c | 4 ---- hw/vfio/region.c | 3 --- hw/xen/xen_pt_msi.c | 11 +---------- 8 files changed, 9 insertions(+), 54 deletions(-)
Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
("[PATCH v2 00/14] hw/pci-host/raven clean ups")
Supersedes: <20240829-memory-v1-1-ac07af2f4fa5@daynix.com>
("[PATCH] docs/devel: Prohibit calling object_unparent() for memory region")
Children are automatically unparented so manually unparenting is
unnecessary.
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.
Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
Changes in v3:
- Added patches to remove other object_unparent() calls in
instance_finalize().
- Dropped patch "qdev: Automatically delete memory subregions" and the
succeeding patches to avoid Ccing many.
- Link to v2: https://lore.kernel.org/qemu-devel/20250915-use-v2-0-f4c7ff13bfe9@rsg.ci.i.u-tokyo.ac.jp
Changes in v2:
- Added a reference to "[PATCH] docs/devel: Prohibit calling
object_unparent() for memory region", which does something similar to
patch "docs/devel: Do not unparent in instance_finalize()" but I
forgot I sent it in the past.
- Fixed a typo in patch
"docs/devel: Do not unparent in instance_finalize()" and
"[PATCH 02/22] vfio/pci: Do not unparent in instance_finalize()".
- Dropped patches to move address_space_init() calls; I intend to
QOM-ify to fix memory leaks automatically as discussed in the
following thread:
https://lore.kernel.org/qemu-devel/cd21698f-db77-eb75-6966-d559fdcab835@eik.bme.hu/
But the QOM-ification will be big so I'll send it as a separate
series.
- Rebased on top of "[PATCH v2 00/14] hw/pci-host/raven clean ups".
https://lore.kernel.org/qemu-devel/cover.1751493467.git.balaton@eik.bme.hu/
- Link to v1: https://lore.kernel.org/qemu-devel/20250906-use-v1-0-c51caafd1eb7@rsg.ci.i.u-tokyo.ac.jp
---
Akihiko Odaki (7):
docs/devel: Do not unparent in instance_finalize()
vfio/pci: Do not unparent in instance_finalize()
hw/core/register: Do not unparent in instance_finalize()
hv-balloon: hw/core/register: Do not unparent in instance_finalize()
hw/sd/sdhci: Do not unparent in instance_finalize()
vfio: Do not unparent in instance_finalize()
hw/xen: Do not unparent in instance_finalize()
docs/devel/memory.rst | 19 ++++++-------------
hw/core/register.c | 1 -
hw/hyperv/hv-balloon.c | 12 +-----------
hw/sd/sdhci.c | 4 ----
hw/vfio/pci-quirks.c | 9 +--------
hw/vfio/pci.c | 4 ----
hw/vfio/region.c | 3 ---
hw/xen/xen_pt_msi.c | 11 +----------
8 files changed, 9 insertions(+), 54 deletions(-)
---
base-commit: e101d33792530093fa0b0a6e5f43e4d8cfe4581e
change-id: 20250906-use-37ecc903a9e0
Best regards,
--
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Queued, thanks. Paolo
On 2025/09/23 17:08, Paolo Bonzini wrote: > Queued, thanks. > > Paolo > I sent v4 to address comments in the following thread: https://lore.kernel.org/qemu-devel/aMxlpfp_LSgiIk9Z@x1.local/ Regards, Akihiko Odaki
On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
Could I ask why this is a dependency?
--
Peter Xu
On Thu, 18 Sep 2025, Peter Xu wrote:
> On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
>> Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
>> ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
>
> Could I ask why this is a dependency?
It removes an address_space usage from raven so this series does not have
to change that and I don't have to rebase that series. Otherwise these are
not related. I'll check the problem reported about my series and send an
updated one.
Regards,
BALATON Zoltan
On Thu, Sep 18, 2025 at 05:29:34PM +0200, BALATON Zoltan wrote:
> On Thu, 18 Sep 2025, Peter Xu wrote:
> > On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> > > Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> >
> > Could I ask why this is a dependency?
>
> It removes an address_space usage from raven so this series does not have to
> change that and I don't have to rebase that series. Otherwise these are not
> related. I'll check the problem reported about my series and send an updated
> one.
This series should be a split of a previous mixed up series that may
contain address space changes while this one doesn't. It also doesn't
touch raven.c and ppc/.
Can I then understand it as the dependency is simply not needed?
Thanks,
--
Peter Xu
On Thu, Sep 18, 2025 at 12:20:49PM -0400, Peter Xu wrote:
> On Thu, Sep 18, 2025 at 05:29:34PM +0200, BALATON Zoltan wrote:
> > On Thu, 18 Sep 2025, Peter Xu wrote:
> > > On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> > > > Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> > > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > >
> > > Could I ask why this is a dependency?
> >
> > It removes an address_space usage from raven so this series does not have to
> > change that and I don't have to rebase that series. Otherwise these are not
> > related. I'll check the problem reported about my series and send an updated
> > one.
>
> This series should be a split of a previous mixed up series that may
> contain address space changes while this one doesn't. It also doesn't
> touch raven.c and ppc/.
>
> Can I then understand it as the dependency is simply not needed?
I meant, it seems we don't need to wait for the other series to merge this
one, hence the there is no real dependency.
I didn't mean to drop that series for sure.. if it was confusing before..
Thanks,
--
Peter Xu
On 2025/09/19 3:23, Peter Xu wrote:
> On Thu, Sep 18, 2025 at 12:20:49PM -0400, Peter Xu wrote:
>> On Thu, Sep 18, 2025 at 05:29:34PM +0200, BALATON Zoltan wrote:
>>> On Thu, 18 Sep 2025, Peter Xu wrote:
>>>> On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
>>>>> Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
>>>>> ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
>>>>
>>>> Could I ask why this is a dependency?
>>>
>>> It removes an address_space usage from raven so this series does not have to
>>> change that and I don't have to rebase that series. Otherwise these are not
>>> related. I'll check the problem reported about my series and send an updated
>>> one.
>>
>> This series should be a split of a previous mixed up series that may
>> contain address space changes while this one doesn't. It also doesn't
>> touch raven.c and ppc/.
>>
>> Can I then understand it as the dependency is simply not needed?
>
> I meant, it seems we don't need to wait for the other series to merge this
> one, hence the there is no real dependency.
You are right. This series can be merged without the raven clean ups.
Regards,
Akihiko Odaki
On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
>
> Supersedes: <20240829-memory-v1-1-ac07af2f4fa5@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 ?
>
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
> Changes in v3:
> - Added patches to remove other object_unparent() calls in
> instance_finalize().
> - Dropped patch "qdev: Automatically delete memory subregions" and the
> succeeding patches to avoid Ccing many.
> - Link to v2: https://lore.kernel.org/qemu-devel/20250915-use-v2-0-f4c7ff13bfe9@rsg.ci.i.u-tokyo.ac.jp
>
> Changes in v2:
> - Added a reference to "[PATCH] docs/devel: Prohibit calling
> object_unparent() for memory region", which does something similar to
> patch "docs/devel: Do not unparent in instance_finalize()" but I
> forgot I sent it in the past.
> - Fixed a typo in patch
> "docs/devel: Do not unparent in instance_finalize()" and
> "[PATCH 02/22] vfio/pci: Do not unparent in instance_finalize()".
> - Dropped patches to move address_space_init() calls; I intend to
> QOM-ify to fix memory leaks automatically as discussed in the
> following thread:
> https://lore.kernel.org/qemu-devel/cd21698f-db77-eb75-6966-d559fdcab835@eik.bme.hu/
> But the QOM-ification will be big so I'll send it as a separate
> series.
> - Rebased on top of "[PATCH v2 00/14] hw/pci-host/raven clean ups".
> https://lore.kernel.org/qemu-devel/cover.1751493467.git.balaton@eik.bme.hu/
> - Link to v1: https://lore.kernel.org/qemu-devel/20250906-use-v1-0-c51caafd1eb7@rsg.ci.i.u-tokyo.ac.jp
>
> ---
> Akihiko Odaki (7):
> docs/devel: Do not unparent in instance_finalize()
> vfio/pci: Do not unparent in instance_finalize()
> hw/core/register: Do not unparent in instance_finalize()
> hv-balloon: hw/core/register: Do not unparent in instance_finalize()
> hw/sd/sdhci: Do not unparent in instance_finalize()
> vfio: Do not unparent in instance_finalize()
> hw/xen: Do not unparent in instance_finalize()
>
> docs/devel/memory.rst | 19 ++++++-------------
> hw/core/register.c | 1 -
> hw/hyperv/hv-balloon.c | 12 +-----------
> hw/sd/sdhci.c | 4 ----
> hw/vfio/pci-quirks.c | 9 +--------
> hw/vfio/pci.c | 4 ----
> hw/vfio/region.c | 3 ---
> hw/xen/xen_pt_msi.c | 11 +----------
> 8 files changed, 9 insertions(+), 54 deletions(-)
> ---
> base-commit: e101d33792530093fa0b0a6e5f43e4d8cfe4581e
> change-id: 20250906-use-37ecc903a9e0
>
> Best regards,
> --
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>
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 :|
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@eik.bme.hu>
>> ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
>>
>> Supersedes: <20240829-memory-v1-1-ac07af2f4fa5@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.
Regards,
Akihiko Odaki
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@eik.bme.hu>
> > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > >
> > > Supersedes: <20240829-memory-v1-1-ac07af2f4fa5@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.
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 :|
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@eik.bme.hu>
> > > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > > >
> > > > Supersedes: <20240829-memory-v1-1-ac07af2f4fa5@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.
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 :|
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.balaton@eik.bme.hu>
> > > > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > > > >
> > > > > Supersedes: <20240829-memory-v1-1-ac07af2f4fa5@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
© 2016 - 2025 Red Hat, Inc.