[PATCH 00/14] Fix memory region use-after-finalization

[PATCH 00/14] Fix memory region use-after-finalization

[Akihiko Odaki]

Based-on: <20250917-use-v3-0-72c2a6887c6c@rsg.ci.i.u-tokyo.ac.jp>
("[PATCH v3 0/7] Do not unparent in instance_finalize()")

This patch series was spun off from "[PATCH v2 00/15] Fix memory region
leaks and use-after-finalization":
https://lore.kernel.org/qemu-devel/20250915-use-v2-0-f4c7ff13bfe9@rsg.ci.i.u-tokyo.ac.jp/

When developing the next version of "[PATCH 00/16] memory: Stop
piggybacking on memory region owners*", I faced multiple memory region
leaks and use-after-finalization. This series extracts their fixes so
that the number of Cc: won't explode.

Patch "qdev: Automatically delete memory subregions" and the succeeding
patches are for refactoring, but patch "vfio-user: Do not delete the
subregion" does fix use-after-finalization.

* https://lore.kernel.org/qemu-devel/20250901-mr-v1-0-dd7cb6b1480b@rsg.ci.i.u-tokyo.ac.jp/

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
Akihiko Odaki (14):
      hw/pci-bridge: Do not assume immediate MemoryRegion finalization
      qdev: Automatically delete memory subregions
      vfio-user: Do not delete the subregion
      hw/char/diva-gsp: Do not delete the subregion
      hw/char/serial-pci-multi: Do not delete the subregion
      secondary-vga: Do not delete the subregions
      cmd646: Do not delete the subregions
      hw/ide/piix: Do not delete the subregions
      hw/ide/via: Do not delete the subregions
      hw/nvme: Do not delete the subregion
      pci: Do not delete the subregions
      hw/ppc/spapr_pci: Do not delete the subregions
      hw/usb/hcd-ehci: Do not delete the subregions
      hw/usb/hcd-xhci: Do not delete the subregions

 MAINTAINERS                |  1 +
 include/hw/pci/pci.h       |  1 +
 include/hw/qdev-core.h     |  1 +
 hw/char/diva-gsp.c         |  1 -
 hw/char/serial-pci-multi.c |  1 -
 hw/core/qdev.c             | 14 +++++++
 hw/display/vga-pci.c       |  8 ----
 hw/ide/cmd646.c            | 12 ------
 hw/ide/piix.c              | 13 -------
 hw/ide/via.c               | 12 ------
 hw/nvme/ctrl.c             |  2 -
 hw/pci/pci.c               | 22 +----------
 hw/pci/pci_bridge.c        | 96 +++++++++++++++++++++++++---------------------
 hw/ppc/spapr_pci.c         | 22 -----------
 hw/usb/hcd-ehci.c          |  4 --
 hw/usb/hcd-xhci.c          | 10 -----
 hw/vfio-user/pci.c         |  6 ---
 stubs/memory.c             |  9 +++++
 stubs/meson.build          |  1 +
 19 files changed, 80 insertions(+), 156 deletions(-)
---
base-commit: e101d33792530093fa0b0a6e5f43e4d8cfe4581e
change-id: 20250917-subregion-907ced7da1ed
prerequisite-change-id: 20250906-use-37ecc903a9e0:v3
prerequisite-patch-id: d464fda86a3c79ff8e6d7a2e623d979b2a47019b
prerequisite-patch-id: 17b153237f69c898b9c5b93aad0d5116d0bfe49f
prerequisite-patch-id: ac51d9c4ac483054ee91cecbb5575def67dbb602
prerequisite-patch-id: 205aa86c0ef087c97dbcf736062661a45c287bf3
prerequisite-patch-id: 26e18a249afaf9cd1b72961f9e2e3ebf97966a3c
prerequisite-patch-id: d3e0b87f84a216e05bd4aa3dee8ae77cf9df062a
prerequisite-patch-id: 510a59304274e1bc35f8fbe77c91fc2f32a2f087

Best regards,
--  
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

[PATCH 01/14] hw/pci-bridge: Do not assume immediate MemoryRegion finalization

[Akihiko Odaki]

When updating memory mappings, pci_bridge_update_mappings() performed
the following operations:
1. Start a transaction
2. Delete the subregions from the container
3. Unparent the subregions
4. Initialize the subregions
5. End the transaction

This assumes the old subregion instances are finalized immediately after
3, but it is not true; the finalization is delayed until 5. Remove the
assumption by using functions to dynamically update MemoryRegions.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 include/hw/pci/pci.h |  1 +
 hw/pci/pci.c         |  2 +-
 hw/pci/pci_bridge.c  | 96 ++++++++++++++++++++++++++++------------------------
 3 files changed, 54 insertions(+), 45 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 6b7d3ac8a361..d0bd214b4e11 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -256,6 +256,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
                       uint8_t attr, MemoryRegion *memory);
 void pci_register_vga(PCIDevice *pci_dev, MemoryRegion *mem,
                       MemoryRegion *io_lo, MemoryRegion *io_hi);
+void pci_update_vga(PCIDevice *pci_dev);
 void pci_unregister_vga(PCIDevice *pci_dev);
 pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index c70b5ceebaf1..516029f66cda 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1521,7 +1521,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     }
 }
 
-static void pci_update_vga(PCIDevice *pci_dev)
+void pci_update_vga(PCIDevice *pci_dev)
 {
     uint16_t cmd;
 
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 76255c4cd892..240d0f904de9 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -145,11 +145,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
     return limit;
 }
 
-static void pci_bridge_init_alias(PCIBridge *bridge, MemoryRegion *alias,
-                                  uint8_t type, const char *name,
-                                  MemoryRegion *space,
-                                  MemoryRegion *parent_space,
-                                  bool enabled)
+static void pci_bridge_update_alias(PCIBridge *bridge, bool init,
+                                    MemoryRegion *alias, uint8_t type,
+                                    const char *name, MemoryRegion *space,
+                                    MemoryRegion *parent_space, bool enabled)
 {
     PCIDevice *bridge_dev = PCI_DEVICE(bridge);
     pcibus_t base = pci_bridge_get_base(bridge_dev, type);
@@ -158,25 +157,37 @@ static void pci_bridge_init_alias(PCIBridge *bridge, MemoryRegion *alias,
      * Apparently no way to do this with existing memory APIs. */
     pcibus_t size = enabled && limit >= base ? limit + 1 - base : 0;
 
-    memory_region_init_alias(alias, OBJECT(bridge), name, space, base, size);
+    if (init) {
+        memory_region_init_alias(alias, OBJECT(bridge), name, space, base, size);
+    } else {
+        memory_region_set_size(alias, size);
+        memory_region_set_alias_offset(alias, base);
+    }
+
     memory_region_add_subregion_overlap(parent_space, base, alias, 1);
 }
 
-static void pci_bridge_init_vga_aliases(PCIBridge *br, PCIBus *parent,
-                                        MemoryRegion *alias_vga)
+static void pci_bridge_update_vga_aliases(PCIBridge *br, bool init,
+                                          PCIBus *parent,
+                                          MemoryRegion *alias_vga)
 {
     PCIDevice *pd = PCI_DEVICE(br);
     uint16_t brctl = pci_get_word(pd->config + PCI_BRIDGE_CONTROL);
 
-    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_LO], OBJECT(br),
-                             "pci_bridge_vga_io_lo", &br->address_space_io,
-                             QEMU_PCI_VGA_IO_LO_BASE, QEMU_PCI_VGA_IO_LO_SIZE);
-    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_HI], OBJECT(br),
-                             "pci_bridge_vga_io_hi", &br->address_space_io,
-                             QEMU_PCI_VGA_IO_HI_BASE, QEMU_PCI_VGA_IO_HI_SIZE);
-    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_MEM], OBJECT(br),
-                             "pci_bridge_vga_mem", &br->address_space_mem,
-                             QEMU_PCI_VGA_MEM_BASE, QEMU_PCI_VGA_MEM_SIZE);
+    if (init) {
+        memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_LO], OBJECT(br),
+                                 "pci_bridge_vga_io_lo", &br->address_space_io,
+                                 QEMU_PCI_VGA_IO_LO_BASE,
+                                 QEMU_PCI_VGA_IO_LO_SIZE);
+        memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_HI], OBJECT(br),
+                                 "pci_bridge_vga_io_hi", &br->address_space_io,
+                                 QEMU_PCI_VGA_IO_HI_BASE,
+                                 QEMU_PCI_VGA_IO_HI_SIZE);
+        memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_MEM], OBJECT(br),
+                                 "pci_bridge_vga_mem", &br->address_space_mem,
+                                 QEMU_PCI_VGA_MEM_BASE,
+                                 QEMU_PCI_VGA_MEM_SIZE);
+    }
 
     if (brctl & PCI_BRIDGE_CTL_VGA) {
         pci_register_vga(pd, &alias_vga[QEMU_PCI_VGA_MEM],
@@ -185,33 +196,33 @@ static void pci_bridge_init_vga_aliases(PCIBridge *br, PCIBus *parent,
     }
 }
 
-static void pci_bridge_region_init(PCIBridge *br)
+static void pci_bridge_region_update(PCIBridge *br, bool init)
 {
     PCIDevice *pd = PCI_DEVICE(br);
     PCIBus *parent = pci_get_bus(pd);
     PCIBridgeWindows *w = &br->windows;
     uint16_t cmd = pci_get_word(pd->config + PCI_COMMAND);
 
-    pci_bridge_init_alias(br, &w->alias_pref_mem,
-                          PCI_BASE_ADDRESS_MEM_PREFETCH,
-                          "pci_bridge_pref_mem",
-                          &br->address_space_mem,
-                          parent->address_space_mem,
-                          cmd & PCI_COMMAND_MEMORY);
-    pci_bridge_init_alias(br, &w->alias_mem,
-                          PCI_BASE_ADDRESS_SPACE_MEMORY,
-                          "pci_bridge_mem",
-                          &br->address_space_mem,
-                          parent->address_space_mem,
-                          cmd & PCI_COMMAND_MEMORY);
-    pci_bridge_init_alias(br, &w->alias_io,
-                          PCI_BASE_ADDRESS_SPACE_IO,
-                          "pci_bridge_io",
-                          &br->address_space_io,
-                          parent->address_space_io,
-                          cmd & PCI_COMMAND_IO);
-
-    pci_bridge_init_vga_aliases(br, parent, w->alias_vga);
+    pci_bridge_update_alias(br, init, &w->alias_pref_mem,
+                            PCI_BASE_ADDRESS_MEM_PREFETCH,
+                            "pci_bridge_pref_mem",
+                            &br->address_space_mem,
+                            parent->address_space_mem,
+                            cmd & PCI_COMMAND_MEMORY);
+    pci_bridge_update_alias(br, init, &w->alias_mem,
+                            PCI_BASE_ADDRESS_SPACE_MEMORY,
+                            "pci_bridge_mem",
+                            &br->address_space_mem,
+                            parent->address_space_mem,
+                            cmd & PCI_COMMAND_MEMORY);
+    pci_bridge_update_alias(br, init, &w->alias_io,
+                            PCI_BASE_ADDRESS_SPACE_IO,
+                            "pci_bridge_io",
+                            &br->address_space_io,
+                            parent->address_space_io,
+                            cmd & PCI_COMMAND_IO);
+
+    pci_bridge_update_vga_aliases(br, init, parent, w->alias_vga);
 }
 
 static void pci_bridge_region_del(PCIBridge *br, PCIBridgeWindows *w)
@@ -237,14 +248,11 @@ static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
 
 void pci_bridge_update_mappings(PCIBridge *br)
 {
-    PCIBridgeWindows *w = &br->windows;
-
     /* Make updates atomic to: handle the case of one VCPU updating the bridge
      * while another accesses an unaffected region. */
     memory_region_transaction_begin();
-    pci_bridge_region_del(br, w);
-    pci_bridge_region_cleanup(br, w);
-    pci_bridge_region_init(br);
+    pci_bridge_region_del(br, &br->windows);
+    pci_bridge_region_update(br, false);
     memory_region_transaction_commit();
 }
 
@@ -386,7 +394,7 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename)
     memory_region_init(&br->address_space_io, OBJECT(br), "pci_bridge_io",
                        4 * GiB);
     address_space_init(&br->as_io, &br->address_space_io, "pci_bridge_pci_io");
-    pci_bridge_region_init(br);
+    pci_bridge_region_update(br, true);
     QLIST_INIT(&sec_bus->child);
     QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
 

-- 
2.51.0

Re: [PATCH 01/14] hw/pci-bridge: Do not assume immediate MemoryRegion finalization

[Peter Xu]

On Wed, Sep 17, 2025 at 07:32:47PM +0900, Akihiko Odaki wrote:
> When updating memory mappings, pci_bridge_update_mappings() performed
> the following operations:
> 1. Start a transaction
> 2. Delete the subregions from the container
> 3. Unparent the subregions
> 4. Initialize the subregions
> 5. End the transaction
> 
> This assumes the old subregion instances are finalized immediately after
> 3, but it is not true; the finalization is delayed until 5. Remove the
> assumption by using functions to dynamically update MemoryRegions.

To Paolo - you can ignore this if you'll merge it.  However I'm still
raising this as a pure question.

Could there be an explanation why it'll be delayed until step 5?

All the MRs involved in this patch are all aliases.  I believe this rules
out any map() ref-takers.  IIUC it is exactly what was marked exceptions in
the memory.rst here:

  There is an exception to the above rule: it is okay to call
  object_unparent at any time for an alias or a container region.  It is
  therefore also okay to create or destroy alias and container regions
  dynamically during a device's lifetime.
  
  This exceptional usage is valid because aliases and containers only help
  QEMU building the guest's memory map; they are never accessed directly.
  memory_region_ref and memory_region_unref are never called on aliases
  or containers, and the above situation then cannot happen.  Exploiting
  this exception is rarely necessary, and therefore it is discouraged,
  but nevertheless it is used in a few places.

I was expecting the current code should at least be fine, no?

Thanks,

-- 
Peter Xu

Re: [PATCH 01/14] hw/pci-bridge: Do not assume immediate MemoryRegion finalization

[Akihiko Odaki]

On 2025/10/03 3:47, Peter Xu wrote:
> On Wed, Sep 17, 2025 at 07:32:47PM +0900, Akihiko Odaki wrote:
>> When updating memory mappings, pci_bridge_update_mappings() performed
>> the following operations:
>> 1. Start a transaction
>> 2. Delete the subregions from the container
>> 3. Unparent the subregions
>> 4. Initialize the subregions
>> 5. End the transaction
>>
>> This assumes the old subregion instances are finalized immediately after
>> 3, but it is not true; the finalization is delayed until 5. Remove the
>> assumption by using functions to dynamically update MemoryRegions.
> 
> To Paolo - you can ignore this if you'll merge it.  However I'm still
> raising this as a pure question.
> 
> Could there be an explanation why it'll be delayed until step 5?
> 
> All the MRs involved in this patch are all aliases.  I believe this rules
> out any map() ref-takers.  IIUC it is exactly what was marked exceptions in
> the memory.rst here:
> 
>    There is an exception to the above rule: it is okay to call
>    object_unparent at any time for an alias or a container region.  It is
>    therefore also okay to create or destroy alias and container regions
>    dynamically during a device's lifetime.
>    
>    This exceptional usage is valid because aliases and containers only help
>    QEMU building the guest's memory map; they are never accessed directly.
>    memory_region_ref and memory_region_unref are never called on aliases
>    or containers, and the above situation then cannot happen.  Exploiting
>    this exception is rarely necessary, and therefore it is discouraged,
>    but nevertheless it is used in a few places.
> 
> I was expecting the current code should at least be fine, no?

You are right. This patch does nothing good so I'll drop it.

Regards,
Akihiko Odaki

[PATCH 02/14] qdev: Automatically delete memory subregions

[Akihiko Odaki]

It is a common requirement of qdev to delete memory subregions to hide
them from address spaces during unrealization. pci automatically
deletes the IO subregions, but this process is manually implemented
in other places, which is tedious and error-prone.

Implement the logic to delete subregions in qdev to cover all devices.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 MAINTAINERS            |  1 +
 include/hw/qdev-core.h |  1 +
 hw/core/qdev.c         | 14 ++++++++++++++
 stubs/memory.c         |  9 +++++++++
 stubs/meson.build      |  1 +
 5 files changed, 26 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8147fff3523e..4665f0a4b7a5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3212,6 +3212,7 @@ F: include/system/memory.h
 F: include/system/ram_addr.h
 F: include/system/ramblock.h
 F: include/system/memory_mapping.h
+F: stubs/memory.c
 F: system/dma-helpers.c
 F: system/ioport.c
 F: system/memory.c
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 530f3da70218..8f443d5f8ea5 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -526,6 +526,7 @@ bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
  *  - unrealize any child buses by calling qbus_unrealize()
  *    (this will recursively unrealize any devices on those buses)
  *  - call the unrealize method of @dev
+ *  - remove @dev from memory
  *
  * The device can then be freed by causing its reference count to go
  * to zero.
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f60022617687..8fdf6774f87e 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -451,6 +451,17 @@ static bool check_only_migratable(Object *obj, Error **errp)
     return true;
 }
 
+static int del_memory_region(Object *child, void *opaque)
+{
+    MemoryRegion *mr = (MemoryRegion *)object_dynamic_cast(child, TYPE_MEMORY_REGION);
+
+    if (mr && mr->container) {
+        memory_region_del_subregion(mr->container, mr);
+    }
+
+    return 0;
+}
+
 static void device_set_realized(Object *obj, bool value, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
@@ -582,6 +593,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         if (dc->unrealize) {
             dc->unrealize(dev);
         }
+        object_child_foreach(OBJECT(dev), del_memory_region, NULL);
         dev->pending_deleted_event = true;
         DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
     }
@@ -606,6 +618,8 @@ post_realize_fail:
     }
 
 fail:
+    object_child_foreach(OBJECT(dev), del_memory_region, NULL);
+
     error_propagate(errp, local_err);
     if (unattached_parent) {
         /*
diff --git a/stubs/memory.c b/stubs/memory.c
new file mode 100644
index 000000000000..9c36531ae542
--- /dev/null
+++ b/stubs/memory.c
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "system/memory.h"
+
+void memory_region_del_subregion(MemoryRegion *mr,
+                                 MemoryRegion *subregion)
+{
+}
diff --git a/stubs/meson.build b/stubs/meson.build
index cef046e6854d..b4df4e60a1af 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -95,5 +95,6 @@ if have_system or have_user
 
   # Also included in have_system for tests/unit/test-qdev-global-props
   stub_ss.add(files('hotplug-stubs.c'))
+  stub_ss.add(files('memory.c'))
   stub_ss.add(files('sysbus.c'))
 endif

-- 
2.51.0

Re: [PATCH 02/14] qdev: Automatically delete memory subregions

[Peter Xu]

On Wed, Sep 17, 2025 at 07:32:48PM +0900, Akihiko Odaki wrote:
> +static int del_memory_region(Object *child, void *opaque)
> +{
> +    MemoryRegion *mr = (MemoryRegion *)object_dynamic_cast(child, TYPE_MEMORY_REGION);
> +
> +    if (mr && mr->container) {
> +        memory_region_del_subregion(mr->container, mr);
> +    }
> +
> +    return 0;
> +}
> +
>  static void device_set_realized(Object *obj, bool value, Error **errp)
>  {
>      DeviceState *dev = DEVICE(obj);
> @@ -582,6 +593,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>          if (dc->unrealize) {
>              dc->unrealize(dev);
>          }
> +        object_child_foreach(OBJECT(dev), del_memory_region, NULL);

PS: I'll keep throwing some pure questions here, again, Paolo - it doesn't
need to block merging if you're confident with the general approach.

Said that, a few things I still want to mention after I read this series..

One thing I really feel hard to review such work is, you hardly describe
the problems the series is resolving.

For example, this patch proposed auto-detach MRs in unrealize() for qdev,
however there's nowhere describing "what will start to work, while it
doesn't", "how bad is the problem", etc..  All the rest patches are about
"what we can avoid do" after this patch.

Meanwhile, the cover letter is misleading. It is:

[PATCH 00/14] Fix memory region use-after-finalization

I believe it's simply wrong, because the whole series is not about
finalize() but unrealize().  For Device class, it also includes the exit()
which will be invoked in pci_qdev_unrealize(), but that is also part of the
unrealize() routine, not finalize().

The other question is, what if a MR has a owner that is not the device
itself?  There's no place enforcing this, hence a qdev can logically have
some sub-objects (which may not really be qdev) that can be the owner of
the memory regions.  Then the device emulation will found that some MRs are
auto-detached and some are not.

One example that I'm aware of is this:

https://lore.kernel.org/all/20250910115420.1012191-2-aesteve@redhat.com/#t

TYPE_VIRTIO_SHARED_MEMORY_MAPPING is an object, not qdev here, which can be
the owner of the MR.

Thanks,

-- 
Peter Xu

Re: [PATCH 02/14] qdev: Automatically delete memory subregions

[Peter Xu]

On Thu, Oct 02, 2025 at 03:23:10PM -0400, Peter Xu wrote:
> On Wed, Sep 17, 2025 at 07:32:48PM +0900, Akihiko Odaki wrote:
> > +static int del_memory_region(Object *child, void *opaque)
> > +{
> > +    MemoryRegion *mr = (MemoryRegion *)object_dynamic_cast(child, TYPE_MEMORY_REGION);
> > +
> > +    if (mr && mr->container) {
> > +        memory_region_del_subregion(mr->container, mr);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static void device_set_realized(Object *obj, bool value, Error **errp)
> >  {
> >      DeviceState *dev = DEVICE(obj);
> > @@ -582,6 +593,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> >          if (dc->unrealize) {
> >              dc->unrealize(dev);
> >          }
> > +        object_child_foreach(OBJECT(dev), del_memory_region, NULL);
> 
> PS: I'll keep throwing some pure questions here, again, Paolo - it doesn't
> need to block merging if you're confident with the general approach.
> 
> Said that, a few things I still want to mention after I read this series..
> 
> One thing I really feel hard to review such work is, you hardly describe
> the problems the series is resolving.
> 
> For example, this patch proposed auto-detach MRs in unrealize() for qdev,
> however there's nowhere describing "what will start to work, while it
> doesn't", "how bad is the problem", etc..  All the rest patches are about
> "what we can avoid do" after this patch.

For this part, I should be more clear on what I'm requesting on the
answers.

I think I get the whole point that MRs (while still with MR refcount
piggypacked, as of current QEMU master does) can circular reference itself
if not always detached properly, so explicitly my question is about:

- What devices / use case you encountered, that QEMU has such issue?
  Especially, this is about after we have merged commit ac7a892fd3 "memory:
  Fix leaks due to owner-shared MRs circular references".  Asking because I
  believe most of them should already auto-detach when owner is shared.

- From above list of broken devices, are there any devices that are
  hot-unpluggable (aka, high priority)?  Is it a problem if we do not
  finalize a MR if it is never removable anyway?

> 
> Meanwhile, the cover letter is misleading. It is:
> 
> [PATCH 00/14] Fix memory region use-after-finalization
> 
> I believe it's simply wrong, because the whole series is not about
> finalize() but unrealize().  For Device class, it also includes the exit()
> which will be invoked in pci_qdev_unrealize(), but that is also part of the
> unrealize() routine, not finalize().
> 
> The other question is, what if a MR has a owner that is not the device
> itself?  There's no place enforcing this, hence a qdev can logically have
> some sub-objects (which may not really be qdev) that can be the owner of
> the memory regions.  Then the device emulation will found that some MRs are
> auto-detached and some are not.
> 
> One example that I'm aware of is this:
> 
> https://lore.kernel.org/all/20250910115420.1012191-2-aesteve@redhat.com/#t
> 
> TYPE_VIRTIO_SHARED_MEMORY_MAPPING is an object, not qdev here, which can be
> the owner of the MR.
> 
> Thanks,
> 
> -- 
> Peter Xu

-- 
Peter Xu

Re: [PATCH 02/14] qdev: Automatically delete memory subregions

[Akihiko Odaki]

On 2025/10/03 4:40, Peter Xu wrote:
> On Thu, Oct 02, 2025 at 03:23:10PM -0400, Peter Xu wrote:
>> On Wed, Sep 17, 2025 at 07:32:48PM +0900, Akihiko Odaki wrote:
>>> +static int del_memory_region(Object *child, void *opaque)
>>> +{
>>> +    MemoryRegion *mr = (MemoryRegion *)object_dynamic_cast(child, TYPE_MEMORY_REGION);
>>> +
>>> +    if (mr && mr->container) {
>>> +        memory_region_del_subregion(mr->container, mr);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static void device_set_realized(Object *obj, bool value, Error **errp)
>>>   {
>>>       DeviceState *dev = DEVICE(obj);
>>> @@ -582,6 +593,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>>           if (dc->unrealize) {
>>>               dc->unrealize(dev);
>>>           }
>>> +        object_child_foreach(OBJECT(dev), del_memory_region, NULL);
>>
>> PS: I'll keep throwing some pure questions here, again, Paolo - it doesn't
>> need to block merging if you're confident with the general approach.
>>
>> Said that, a few things I still want to mention after I read this series..
>>
>> One thing I really feel hard to review such work is, you hardly describe
>> the problems the series is resolving.
>>
>> For example, this patch proposed auto-detach MRs in unrealize() for qdev,
>> however there's nowhere describing "what will start to work, while it
>> doesn't", "how bad is the problem", etc..  All the rest patches are about
>> "what we can avoid do" after this patch.
> 
> For this part, I should be more clear on what I'm requesting on the
> answers.
> 
> I think I get the whole point that MRs (while still with MR refcount
> piggypacked, as of current QEMU master does) can circular reference itself
> if not always detached properly, so explicitly my question is about:
> 
> - What devices / use case you encountered, that QEMU has such issue?
>    Especially, this is about after we have merged commit ac7a892fd3 "memory:
>    Fix leaks due to owner-shared MRs circular references".  Asking because I
>    believe most of them should already auto-detach when owner is shared.
> 
> - From above list of broken devices, are there any devices that are
>    hot-unpluggable (aka, high priority)?  Is it a problem if we do not
>    finalize a MR if it is never removable anyway?
> 
>>
>> Meanwhile, the cover letter is misleading. It is:
>>
>> [PATCH 00/14] Fix memory region use-after-finalization
>>
>> I believe it's simply wrong, because the whole series is not about
>> finalize() but unrealize().  For Device class, it also includes the exit()
>> which will be invoked in pci_qdev_unrealize(), but that is also part of the
>> unrealize() routine, not finalize().

The subject of the cover letter "fix memory region 
use-after-finalization" is confusing. While this series has such fixes, 
it also contain refactoring patches. The cover letter says:

 > Patch "qdev: Automatically delete memory subregions" and the
 > succeeding patches are for refactoring, but patch "vfio-user: Do not
 > delete the subregion" does fix use-after-finalization.

More concretely, patch "qdev: Automatically delete memory subregions" 
implements a common pattern of device unrealization, and the suceeding 
patches remove ad-hoc implementations of it.

And since patch "hw/pci-bridge: Do not assume immediate MemoryRegion 
finalization" fixes nothing as you pointed out, only patch "vfio-user: 
Do not delete the subregion" fixes something.

Without patch "vfio-user: Do not delete the subregion", 
vfio_user_msix_teardown() calls memory_region_del_subregion(). However, 
this function is called from instance_finalize, so the subregion is 
already finalized and results in a use-after-finalization scenario.

Anything else is for refactoring and it's quite unlike patch "memory: 
Fix leaks due to owner-shared MRs circular references", which is a bug fix.

I think I'll drop patch "hw/pci-bridge: Do not assume immediate 
MemoryRegion finalization" and rename this series simply to "qdev: 
Automatically delete memory subregions" to avoid confusion.

>>
>> The other question is, what if a MR has a owner that is not the device
>> itself?  There's no place enforcing this, hence a qdev can logically have
>> some sub-objects (which may not really be qdev) that can be the owner of
>> the memory regions.  Then the device emulation will found that some MRs are
>> auto-detached and some are not.
>>
>> One example that I'm aware of is this:
>>
>> https://lore.kernel.org/all/20250910115420.1012191-2-aesteve@redhat.com/#t
>>
>> TYPE_VIRTIO_SHARED_MEMORY_MAPPING is an object, not qdev here, which can be
>> the owner of the MR.

Patch "qdev: Automatically delete memory subregions" and the succeeding 
patches are for refactoring of qdev. MRs not directly owned by qdev are 
out of scope of the change.

Regards,
Akihiko Odaki

Re: [PATCH 02/14] qdev: Automatically delete memory subregions

[Peter Xu]

On Fri, Oct 03, 2025 at 11:01:38PM +0900, Akihiko Odaki wrote:
> On 2025/10/03 4:40, Peter Xu wrote:
> > On Thu, Oct 02, 2025 at 03:23:10PM -0400, Peter Xu wrote:
> > > On Wed, Sep 17, 2025 at 07:32:48PM +0900, Akihiko Odaki wrote:
> > > > +static int del_memory_region(Object *child, void *opaque)
> > > > +{
> > > > +    MemoryRegion *mr = (MemoryRegion *)object_dynamic_cast(child, TYPE_MEMORY_REGION);
> > > > +
> > > > +    if (mr && mr->container) {
> > > > +        memory_region_del_subregion(mr->container, mr);
> > > > +    }
> > > > +
> > > > +    return 0;
> > > > +}
> > > > +
> > > >   static void device_set_realized(Object *obj, bool value, Error **errp)
> > > >   {
> > > >       DeviceState *dev = DEVICE(obj);
> > > > @@ -582,6 +593,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
> > > >           if (dc->unrealize) {
> > > >               dc->unrealize(dev);
> > > >           }
> > > > +        object_child_foreach(OBJECT(dev), del_memory_region, NULL);
> > > 
> > > PS: I'll keep throwing some pure questions here, again, Paolo - it doesn't
> > > need to block merging if you're confident with the general approach.
> > > 
> > > Said that, a few things I still want to mention after I read this series..
> > > 
> > > One thing I really feel hard to review such work is, you hardly describe
> > > the problems the series is resolving.
> > > 
> > > For example, this patch proposed auto-detach MRs in unrealize() for qdev,
> > > however there's nowhere describing "what will start to work, while it
> > > doesn't", "how bad is the problem", etc..  All the rest patches are about
> > > "what we can avoid do" after this patch.
> > 
> > For this part, I should be more clear on what I'm requesting on the
> > answers.
> > 
> > I think I get the whole point that MRs (while still with MR refcount
> > piggypacked, as of current QEMU master does) can circular reference itself
> > if not always detached properly, so explicitly my question is about:
> > 
> > - What devices / use case you encountered, that QEMU has such issue?
> >    Especially, this is about after we have merged commit ac7a892fd3 "memory:
> >    Fix leaks due to owner-shared MRs circular references".  Asking because I
> >    believe most of them should already auto-detach when owner is shared.
> > 
> > - From above list of broken devices, are there any devices that are
> >    hot-unpluggable (aka, high priority)?  Is it a problem if we do not
> >    finalize a MR if it is never removable anyway?

[1]

> > 
> > > 
> > > Meanwhile, the cover letter is misleading. It is:
> > > 
> > > [PATCH 00/14] Fix memory region use-after-finalization
> > > 
> > > I believe it's simply wrong, because the whole series is not about
> > > finalize() but unrealize().  For Device class, it also includes the exit()
> > > which will be invoked in pci_qdev_unrealize(), but that is also part of the
> > > unrealize() routine, not finalize().
> 
> The subject of the cover letter "fix memory region use-after-finalization"
> is confusing. While this series has such fixes, it also contain refactoring
> patches. The cover letter says:
> 
> > Patch "qdev: Automatically delete memory subregions" and the
> > succeeding patches are for refactoring, but patch "vfio-user: Do not
> > delete the subregion" does fix use-after-finalization.
> 
> More concretely, patch "qdev: Automatically delete memory subregions"
> implements a common pattern of device unrealization, and the suceeding
> patches remove ad-hoc implementations of it.
> 
> And since patch "hw/pci-bridge: Do not assume immediate MemoryRegion
> finalization" fixes nothing as you pointed out, only patch "vfio-user: Do
> not delete the subregion" fixes something.
> 
> Without patch "vfio-user: Do not delete the subregion",
> vfio_user_msix_teardown() calls memory_region_del_subregion(). However, this
> function is called from instance_finalize, so the subregion is already
> finalized and results in a use-after-finalization scenario.
> 
> Anything else is for refactoring and it's quite unlike patch "memory: Fix
> leaks due to owner-shared MRs circular references", which is a bug fix.
> 
> I think I'll drop patch "hw/pci-bridge: Do not assume immediate MemoryRegion
> finalization" and rename this series simply to "qdev: Automatically delete
> memory subregions" to avoid confusion.

Yes, thanks.  I went over quite a few follow up patches but I missed this
one.  IMHO you can also split the only fix out, so that can be better
looked at by vfio-user developers.  It'll also be easier for them to verify
if they want.

> 
> > > 
> > > The other question is, what if a MR has a owner that is not the device
> > > itself?  There's no place enforcing this, hence a qdev can logically have
> > > some sub-objects (which may not really be qdev) that can be the owner of
> > > the memory regions.  Then the device emulation will found that some MRs are
> > > auto-detached and some are not.
> > > 
> > > One example that I'm aware of is this:
> > > 
> > > https://lore.kernel.org/all/20250910115420.1012191-2-aesteve@redhat.com/#t
> > > 
> > > TYPE_VIRTIO_SHARED_MEMORY_MAPPING is an object, not qdev here, which can be
> > > the owner of the MR.
> 
> Patch "qdev: Automatically delete memory subregions" and the succeeding
> patches are for refactoring of qdev. MRs not directly owned by qdev are out
> of scope of the change.

Do you have a rough answer of above question [1], on what might suffer from
lost MRs?  I sincerely want to know how much we are missing after we could
already auto-detach owner-shared MRs.

From a quick glance, at least patch 4-14 are detaching MRs that shares the
same owner.  IIUC, it means at least patch 4-14 do not rely on patch 2.

Then I wonder how much patch 2 helps in real life.

There's indeed a difference though when a qdev may realize(), unrealize()
and realize() in a sequence, in which case patch 2 could help whil commit
ac7a892fd3 won't, however I don't know whether there's real use case,
either.

I also wished if there's such device, it'll have explicit detach code so
that when I debug a problem on the device I can easily see when the MRs
will be available, instead of remembering all the rules when something in
some layer will auto-detach...

Personally I think such automation adds burden to developers' mind if
there're still a bunch of MRs that are not used in this way (there
definitely are, otherwise we should be able to completely unexport
memory_region_del_subregion). But that's subjective; I believe at least
Paolo agrees with your general approach.

Thanks,

-- 
Peter Xu

Re: [PATCH 02/14] qdev: Automatically delete memory subregions

[Akihiko Odaki]

On 2025/10/04 0:26, Peter Xu wrote:
> On Fri, Oct 03, 2025 at 11:01:38PM +0900, Akihiko Odaki wrote:
>> On 2025/10/03 4:40, Peter Xu wrote:
>>> On Thu, Oct 02, 2025 at 03:23:10PM -0400, Peter Xu wrote:
>>>> On Wed, Sep 17, 2025 at 07:32:48PM +0900, Akihiko Odaki wrote:
>>>>> +static int del_memory_region(Object *child, void *opaque)
>>>>> +{
>>>>> +    MemoryRegion *mr = (MemoryRegion *)object_dynamic_cast(child, TYPE_MEMORY_REGION);
>>>>> +
>>>>> +    if (mr && mr->container) {
>>>>> +        memory_region_del_subregion(mr->container, mr);
>>>>> +    }
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>    static void device_set_realized(Object *obj, bool value, Error **errp)
>>>>>    {
>>>>>        DeviceState *dev = DEVICE(obj);
>>>>> @@ -582,6 +593,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
>>>>>            if (dc->unrealize) {
>>>>>                dc->unrealize(dev);
>>>>>            }
>>>>> +        object_child_foreach(OBJECT(dev), del_memory_region, NULL);
>>>>
>>>> PS: I'll keep throwing some pure questions here, again, Paolo - it doesn't
>>>> need to block merging if you're confident with the general approach.
>>>>
>>>> Said that, a few things I still want to mention after I read this series..
>>>>
>>>> One thing I really feel hard to review such work is, you hardly describe
>>>> the problems the series is resolving.
>>>>
>>>> For example, this patch proposed auto-detach MRs in unrealize() for qdev,
>>>> however there's nowhere describing "what will start to work, while it
>>>> doesn't", "how bad is the problem", etc..  All the rest patches are about
>>>> "what we can avoid do" after this patch.
>>>
>>> For this part, I should be more clear on what I'm requesting on the
>>> answers.
>>>
>>> I think I get the whole point that MRs (while still with MR refcount
>>> piggypacked, as of current QEMU master does) can circular reference itself
>>> if not always detached properly, so explicitly my question is about:
>>>
>>> - What devices / use case you encountered, that QEMU has such issue?
>>>     Especially, this is about after we have merged commit ac7a892fd3 "memory:
>>>     Fix leaks due to owner-shared MRs circular references".  Asking because I
>>>     believe most of them should already auto-detach when owner is shared.
>>>
>>> - From above list of broken devices, are there any devices that are
>>>     hot-unpluggable (aka, high priority)?  Is it a problem if we do not
>>>     finalize a MR if it is never removable anyway?
> 
> [1]
> 
>>>
>>>>
>>>> Meanwhile, the cover letter is misleading. It is:
>>>>
>>>> [PATCH 00/14] Fix memory region use-after-finalization
>>>>
>>>> I believe it's simply wrong, because the whole series is not about
>>>> finalize() but unrealize().  For Device class, it also includes the exit()
>>>> which will be invoked in pci_qdev_unrealize(), but that is also part of the
>>>> unrealize() routine, not finalize().
>>
>> The subject of the cover letter "fix memory region use-after-finalization"
>> is confusing. While this series has such fixes, it also contain refactoring
>> patches. The cover letter says:
>>
>>> Patch "qdev: Automatically delete memory subregions" and the
>>> succeeding patches are for refactoring, but patch "vfio-user: Do not
>>> delete the subregion" does fix use-after-finalization.
>>
>> More concretely, patch "qdev: Automatically delete memory subregions"
>> implements a common pattern of device unrealization, and the suceeding
>> patches remove ad-hoc implementations of it.
>>
>> And since patch "hw/pci-bridge: Do not assume immediate MemoryRegion
>> finalization" fixes nothing as you pointed out, only patch "vfio-user: Do
>> not delete the subregion" fixes something.
>>
>> Without patch "vfio-user: Do not delete the subregion",
>> vfio_user_msix_teardown() calls memory_region_del_subregion(). However, this
>> function is called from instance_finalize, so the subregion is already
>> finalized and results in a use-after-finalization scenario.
>>
>> Anything else is for refactoring and it's quite unlike patch "memory: Fix
>> leaks due to owner-shared MRs circular references", which is a bug fix.
>>
>> I think I'll drop patch "hw/pci-bridge: Do not assume immediate MemoryRegion
>> finalization" and rename this series simply to "qdev: Automatically delete
>> memory subregions" to avoid confusion.
> 
> Yes, thanks.  I went over quite a few follow up patches but I missed this
> one.  IMHO you can also split the only fix out, so that can be better
> looked at by vfio-user developers.  It'll also be easier for them to verify
> if they want.

I'll split the VFIO patch out since I found patch "qdev: Automatically 
delete memory subregions" is unnecessary; I'll describe that later.

> 
>>
>>>>
>>>> The other question is, what if a MR has a owner that is not the device
>>>> itself?  There's no place enforcing this, hence a qdev can logically have
>>>> some sub-objects (which may not really be qdev) that can be the owner of
>>>> the memory regions.  Then the device emulation will found that some MRs are
>>>> auto-detached and some are not.
>>>>
>>>> One example that I'm aware of is this:
>>>>
>>>> https://lore.kernel.org/all/20250910115420.1012191-2-aesteve@redhat.com/#t
>>>>
>>>> TYPE_VIRTIO_SHARED_MEMORY_MAPPING is an object, not qdev here, which can be
>>>> the owner of the MR.
>>
>> Patch "qdev: Automatically delete memory subregions" and the succeeding
>> patches are for refactoring of qdev. MRs not directly owned by qdev are out
>> of scope of the change.
> 
> Do you have a rough answer of above question [1], on what might suffer from
> lost MRs?  I sincerely want to know how much we are missing after we could
> already auto-detach owner-shared MRs.

There is no functionally "broken" device. The patch that does fix 
something is the VFIO patch, and it fixes use-after-finalization, which 
is semantically problematic but does not cause a functional problem 
(like use-after-free).

> 
>  From a quick glance, at least patch 4-14 are detaching MRs that shares the
> same owner.  IIUC, it means at least patch 4-14 do not rely on patch 2.

If the container and the subregion shares the same owner, 
memory_region_del_subregion() is unnecessary in the first place and can 
be removed even without patch 2, and it seems there are a number of such 
unnecessary memory_region_del_subregion() calls.

That said, there are cases where memory_region_del_subregion() is truly 
needed. In general, a device exposes its MMIO functionality to the guest 
by adding its memory region to external containers, so there should be 
some containers and subregions that do not share memory regions.

Reviewing removed memory_region_del_subregion() calls in each patch, 
patch 3, 6, 7, 8, 9, 10, 13, and 14 are for memory regions with shared 
owners (that's why patch 3 can be applied without the qdev patch).
Patch 4, 5, 11 and 12 are for memory regions with different owners.

The point here is that we don't have to care whether memory regions 
share owners if we delete all subregions for defensive programming.

> 
> Then I wonder how much patch 2 helps in real life.
> 
> There's indeed a difference though when a qdev may realize(), unrealize()
> and realize() in a sequence, in which case patch 2 could help whil commit
> ac7a892fd3 won't, however I don't know whether there's real use case,
> either.

It is not relevant what order realize() and unrealize() are performed. 
This patch is to de-duplicate the code to unrealize devices, and commit 
ac7a892fd3 fixes a bug instead.

> 
> I also wished if there's such device, it'll have explicit detach code so
> that when I debug a problem on the device I can easily see when the MRs
> will be available, instead of remembering all the rules when something in
> some layer will auto-detach...

Indeed, people who read the implementation of devices wonder where are 
memory_region_del_subregion() calls that corresponding to 
memory_region_add_subregion() calls they see.

That said, anyone who cares device code already needs to know that 
subregions (that do not share owners) need to be deleted during 
unrealization, or devices will remain visible to the guest. I see 
avoiding this consequence is important, and it is just nice to have less 
code by de-deuplicating the memory_del_subregion() calls.

> 
> Personally I think such automation adds burden to developers' mind if
> there're still a bunch of MRs that are not used in this way (there
> definitely are, otherwise we should be able to completely unexport
> memory_region_del_subregion). But that's subjective; I believe at least
> Paolo agrees with your general approach.

Indeed, memory_region_del_subregion() is still necessary for memory 
regions that are not directly owned by devices. This is a trade-off 
situation so there is no clear answer what's the best.

Regards,
Akihiko Odaki

[PATCH 03/14] vfio-user: Do not delete the subregion

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/vfio-user/pci.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/vfio-user/pci.c b/hw/vfio-user/pci.c
index be71c777291f..0b6c6a1c5ed3 100644
--- a/hw/vfio-user/pci.c
+++ b/hw/vfio-user/pci.c
@@ -73,12 +73,6 @@ static void vfio_user_msix_setup(VFIOPCIDevice *vdev)
 
 static void vfio_user_msix_teardown(VFIOPCIDevice *vdev)
 {
-    MemoryRegion *mr, *sub;
-
-    mr = vdev->bars[vdev->msix->pba_bar].mr;
-    sub = vdev->msix->pba_region;
-    memory_region_del_subregion(mr, sub);
-
     g_free(vdev->msix->pba_region);
     vdev->msix->pba_region = NULL;
 }

-- 
2.51.0

[PATCH 04/14] hw/char/diva-gsp: Do not delete the subregion

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/char/diva-gsp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/char/diva-gsp.c b/hw/char/diva-gsp.c
index e1f0713cb794..1ae472e879b5 100644
--- a/hw/char/diva-gsp.c
+++ b/hw/char/diva-gsp.c
@@ -63,7 +63,6 @@ static void diva_pci_exit(PCIDevice *dev)
     for (i = 0; i < pci->ports; i++) {
         s = pci->state + i;
         qdev_unrealize(DEVICE(s));
-        memory_region_del_subregion(&pci->membar, &s->io);
         g_free(pci->name[i]);
     }
     qemu_free_irqs(pci->irqs, pci->ports);

-- 
2.51.0

[PATCH 05/14] hw/char/serial-pci-multi: Do not delete the subregion

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/char/serial-pci-multi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
index 13df272691a6..3132ca90cebf 100644
--- a/hw/char/serial-pci-multi.c
+++ b/hw/char/serial-pci-multi.c
@@ -57,7 +57,6 @@ static void multi_serial_pci_exit(PCIDevice *dev)
     for (i = 0; i < pci->ports; i++) {
         s = pci->state + i;
         qdev_unrealize(DEVICE(s));
-        memory_region_del_subregion(&pci->iobar, &s->io);
         g_free(pci->name[i]);
     }
 }

-- 
2.51.0

[PATCH 06/14] secondary-vga: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/display/vga-pci.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index b81f7fd2d0fd..90b4545d3821 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -307,14 +307,6 @@ static void pci_secondary_vga_exit(PCIDevice *dev)
     VGACommonState *s = &d->vga;
 
     graphic_console_close(s->con);
-    memory_region_del_subregion(&d->mmio, &d->mrs[0]);
-    memory_region_del_subregion(&d->mmio, &d->mrs[1]);
-    if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
-        memory_region_del_subregion(&d->mmio, &d->mrs[2]);
-    }
-    if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_EDID)) {
-        memory_region_del_subregion(&d->mmio, &d->mrs[3]);
-    }
 }
 
 static void pci_secondary_vga_init(Object *obj)

-- 
2.51.0

[PATCH 07/14] cmd646: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/ide/cmd646.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 2a59516a9ddb..ea4d501c5e40 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -302,17 +302,6 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
     }
 }
 
-static void pci_cmd646_ide_exitfn(PCIDevice *dev)
-{
-    PCIIDEState *d = PCI_IDE(dev);
-    unsigned i;
-
-    for (i = 0; i < 2; ++i) {
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
-    }
-}
-
 static const Property cmd646_ide_properties[] = {
     DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
 };
@@ -325,7 +314,6 @@ static void cmd646_ide_class_init(ObjectClass *klass, const void *data)
     device_class_set_legacy_reset(dc, cmd646_reset);
     dc->vmsd = &vmstate_ide_pci;
     k->realize = pci_cmd646_ide_realize;
-    k->exit = pci_cmd646_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_CMD;
     k->device_id = PCI_DEVICE_ID_CMD_646;
     k->revision = 0x07;

-- 
2.51.0

[PATCH 08/14] hw/ide/piix: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/ide/piix.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a0f2709c6973..138f8e1936b4 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -166,17 +166,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
     }
 }
 
-static void pci_piix_ide_exitfn(PCIDevice *dev)
-{
-    PCIIDEState *d = PCI_IDE(dev);
-    unsigned i;
-
-    for (i = 0; i < 2; ++i) {
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
-    }
-}
-
 /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 static void piix3_ide_class_init(ObjectClass *klass, const void *data)
 {
@@ -186,7 +175,6 @@ static void piix3_ide_class_init(ObjectClass *klass, const void *data)
     device_class_set_legacy_reset(dc, piix_ide_reset);
     dc->vmsd = &vmstate_ide_pci;
     k->realize = pci_piix_ide_realize;
-    k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
     k->class_id = PCI_CLASS_STORAGE_IDE;
@@ -209,7 +197,6 @@ static void piix4_ide_class_init(ObjectClass *klass, const void *data)
     device_class_set_legacy_reset(dc, piix_ide_reset);
     dc->vmsd = &vmstate_ide_pci;
     k->realize = pci_piix_ide_realize;
-    k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
     k->class_id = PCI_CLASS_STORAGE_IDE;

-- 
2.51.0

[PATCH 09/14] hw/ide/via: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/ide/via.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index dedc2674c002..cbaf4ad1548b 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -234,17 +234,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     }
 }
 
-static void via_ide_exitfn(PCIDevice *dev)
-{
-    PCIIDEState *d = PCI_IDE(dev);
-    unsigned i;
-
-    for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
-    }
-}
-
 static void via_ide_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -258,7 +247,6 @@ static void via_ide_class_init(ObjectClass *klass, const void *data)
     k->config_read = via_ide_cfg_read;
     k->config_write = via_ide_cfg_write;
     k->realize = via_ide_realize;
-    k->exit = via_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_VIA;
     k->device_id = PCI_DEVICE_ID_VIA_IDE;
     k->revision = 0x06;

-- 
2.51.0

[PATCH 10/14] hw/nvme: Do not delete the subregion

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/nvme/ctrl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index f5ee6bf260f1..eebce1f787f4 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -9058,8 +9058,6 @@ static void nvme_exit(PCIDevice *pci_dev)
     } else {
         msix_uninit(pci_dev, &n->bar0, &n->bar0);
     }
-
-    memory_region_del_subregion(&n->bar0, &n->iomem);
 }
 
 static const Property nvme_props[] = {

-- 
2.51.0

[PATCH 11/14] pci: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/pci/pci.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 516029f66cda..2b408c7ec336 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1188,10 +1188,6 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
     if (xen_mode == XEN_EMULATE) {
         xen_evtchn_remove_pci_device(pci_dev);
     }
-    if (memory_region_is_mapped(&pci_dev->bus_master_enable_region)) {
-        memory_region_del_subregion(&pci_dev->bus_master_container_region,
-                                    &pci_dev->bus_master_enable_region);
-    }
     address_space_destroy(&pci_dev->bus_master_as);
 }
 
@@ -1417,27 +1413,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
     return pci_dev;
 }
 
-static void pci_unregister_io_regions(PCIDevice *pci_dev)
-{
-    PCIIORegion *r;
-    int i;
-
-    for(i = 0; i < PCI_NUM_REGIONS; i++) {
-        r = &pci_dev->io_regions[i];
-        if (!r->size || r->addr == PCI_BAR_UNMAPPED)
-            continue;
-        memory_region_del_subregion(r->address_space, r->memory);
-    }
-
-    pci_unregister_vga(pci_dev);
-}
-
 static void pci_qdev_unrealize(DeviceState *dev)
 {
     PCIDevice *pci_dev = PCI_DEVICE(dev);
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
 
-    pci_unregister_io_regions(pci_dev);
     pci_del_option_rom(pci_dev);
     pcie_sriov_unregister_device(pci_dev);
 

-- 
2.51.0

[PATCH 12/14] hw/ppc/spapr_pci: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/ppc/spapr_pci.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 1ac1185825e8..b4043ee752c5 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1735,27 +1735,13 @@ static void spapr_phb_unrealize(DeviceState *dev)
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
     SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(phb);
-    SpaprTceTable *tcet;
     int i;
-    const unsigned windows_supported = spapr_phb_windows_supported(sphb);
 
     if (sphb->msi) {
         g_hash_table_unref(sphb->msi);
         sphb->msi = NULL;
     }
 
-    /*
-     * Remove IO/MMIO subregions and aliases, rest should get cleaned
-     * via PHB's unrealize->object_finalize
-     */
-    for (i = windows_supported - 1; i >= 0; i--) {
-        tcet = spapr_tce_find_by_liobn(sphb->dma_liobn[i]);
-        if (tcet) {
-            memory_region_del_subregion(&sphb->iommu_root,
-                                        spapr_tce_get_iommu(tcet));
-        }
-    }
-
     remove_drcs(sphb, phb->bus);
 
     for (i = PCI_NUM_PINS - 1; i >= 0; i--) {
@@ -1767,8 +1753,6 @@ static void spapr_phb_unrealize(DeviceState *dev)
 
     QLIST_REMOVE(sphb, list);
 
-    memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow);
-
     /*
      * An attached PCI device may have memory listeners, eg. VFIO PCI. We have
      * unmapped all sections. Remove the listeners now, before destroying the
@@ -1779,12 +1763,6 @@ static void spapr_phb_unrealize(DeviceState *dev)
 
     qbus_set_hotplug_handler(BUS(phb->bus), NULL);
     pci_unregister_root_bus(phb->bus);
-
-    memory_region_del_subregion(get_system_memory(), &sphb->iowindow);
-    if (sphb->mem64_win_pciaddr != (hwaddr)-1) {
-        memory_region_del_subregion(get_system_memory(), &sphb->mem64window);
-    }
-    memory_region_del_subregion(get_system_memory(), &sphb->mem32window);
 }
 
 static void spapr_phb_destroy_msi(gpointer opaque)

-- 
2.51.0

[PATCH 13/14] hw/usb/hcd-ehci: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/usb/hcd-ehci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index b090f253656b..21c3501455b5 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2557,10 +2557,6 @@ void usb_ehci_unrealize(EHCIState *s, DeviceState *dev)
     ehci_queues_rip_all(s, 0);
     ehci_queues_rip_all(s, 1);
 
-    memory_region_del_subregion(&s->mem, &s->mem_caps);
-    memory_region_del_subregion(&s->mem, &s->mem_opreg);
-    memory_region_del_subregion(&s->mem, &s->mem_ports);
-
     usb_bus_release(&s->bus);
 
     if (s->vmstate) {

-- 
2.51.0

[PATCH 14/14] hw/usb/hcd-xhci: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/usb/hcd-xhci.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 292c378bfc98..b68a2aec3171 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3454,16 +3454,6 @@ static void usb_xhci_unrealize(DeviceState *dev)
         xhci->mfwrap_timer = NULL;
     }
 
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_cap);
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_oper);
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_runtime);
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_doorbell);
-
-    for (i = 0; i < xhci->numports; i++) {
-        XHCIPort *port = &xhci->ports[i];
-        memory_region_del_subregion(&xhci->mem, &port->mem);
-    }
-
     usb_bus_release(&xhci->bus);
 }
 

-- 
2.51.0

Re: [PATCH 00/14] Fix memory region use-after-finalization

[Paolo Bonzini]

On 9/17/25 12:32, Akihiko Odaki wrote:
> Based-on: <20250917-use-v3-0-72c2a6887c6c@rsg.ci.i.u-tokyo.ac.jp>
> ("[PATCH v3 0/7] Do not unparent in instance_finalize()")
> 
> This patch series was spun off from "[PATCH v2 00/15] Fix memory region
> leaks and use-after-finalization":
> https://lore.kernel.org/qemu-devel/20250915-use-v2-0-f4c7ff13bfe9@rsg.ci.i.u-tokyo.ac.jp/
> 
> When developing the next version of "[PATCH 00/16] memory: Stop
> piggybacking on memory region owners*", I faced multiple memory region
> leaks and use-after-finalization. This series extracts their fixes so
> that the number of Cc: won't explode.
> 
> Patch "qdev: Automatically delete memory subregions" and the succeeding
> patches are for refactoring, but patch "vfio-user: Do not delete the
> subregion" does fix use-after-finalization.
> 
> * https://lore.kernel.org/qemu-devel/20250901-mr-v1-0-dd7cb6b1480b@rsg.ci.i.u-tokyo.ac.jp/
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

This makes sense, but I think it is not bisectable, because of this in 
memory_region_del_subregion():

     assert(subregion->container == mr);
     subregion->container = NULL;

You would need to add a temporary

     if (subregion->container == NULL) {
         return;
     }

and undo it at the end of the series.  Do you agree?  With this change I 
can apply it.

Paolo

Re: [PATCH 00/14] Fix memory region use-after-finalization

[Akihiko Odaki]

On 2025/10/03 0:03, Paolo Bonzini wrote:
> On 9/17/25 12:32, Akihiko Odaki wrote:
>> Based-on: <20250917-use-v3-0-72c2a6887c6c@rsg.ci.i.u-tokyo.ac.jp>
>> ("[PATCH v3 0/7] Do not unparent in instance_finalize()")
>>
>> This patch series was spun off from "[PATCH v2 00/15] Fix memory region
>> leaks and use-after-finalization":
>> https://lore.kernel.org/qemu-devel/20250915-use-v2-0- 
>> f4c7ff13bfe9@rsg.ci.i.u-tokyo.ac.jp/
>>
>> When developing the next version of "[PATCH 00/16] memory: Stop
>> piggybacking on memory region owners*", I faced multiple memory region
>> leaks and use-after-finalization. This series extracts their fixes so
>> that the number of Cc: won't explode.
>>
>> Patch "qdev: Automatically delete memory subregions" and the succeeding
>> patches are for refactoring, but patch "vfio-user: Do not delete the
>> subregion" does fix use-after-finalization.
>>
>> * https://lore.kernel.org/qemu-devel/20250901-mr-v1-0- 
>> dd7cb6b1480b@rsg.ci.i.u-tokyo.ac.jp/
>>
>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> 
> This makes sense, but I think it is not bisectable, because of this in 
> memory_region_del_subregion():
> 
>      assert(subregion->container == mr);
>      subregion->container = NULL;
> 
> You would need to add a temporary
> 
>      if (subregion->container == NULL) {
>          return;
>      }
> 
> and undo it at the end of the series.  Do you agree?  With this change I 
> can apply it.

It is unnecessary because patch "qdev: Automatically delete memory 
subregions" satisfies the following:

1. the device-specific code can assume that subregions they added are 
present until it finishes unrealization. The unrealize() callback can 
also assume the subregions are present and delete them. qdev satisfies 
this by deleting subregions only after calling the unrealize().

2. qdev should delete the remaining subregions before it finishes 
unrealization to ensure that the devices are hidden from the guest. qdev 
satisfies this by checking if memory regions have containers before 
deleting.

Regards,
Akihiko Odaki

Re: [PATCH 00/14] Fix memory region use-after-finalization

[Peter Xu]

On Fri, Oct 10, 2025 at 07:20:04PM +0900, Akihiko Odaki wrote:
> On 2025/10/03 0:03, Paolo Bonzini wrote:
> > On 9/17/25 12:32, Akihiko Odaki wrote:
> > > Based-on: <20250917-use-v3-0-72c2a6887c6c@rsg.ci.i.u-tokyo.ac.jp>
> > > ("[PATCH v3 0/7] Do not unparent in instance_finalize()")
> > > 
> > > This patch series was spun off from "[PATCH v2 00/15] Fix memory region
> > > leaks and use-after-finalization":
> > > https://lore.kernel.org/qemu-devel/20250915-use-v2-0-
> > > f4c7ff13bfe9@rsg.ci.i.u-tokyo.ac.jp/
> > > 
> > > When developing the next version of "[PATCH 00/16] memory: Stop
> > > piggybacking on memory region owners*", I faced multiple memory region
> > > leaks and use-after-finalization. This series extracts their fixes so
> > > that the number of Cc: won't explode.
> > > 
> > > Patch "qdev: Automatically delete memory subregions" and the succeeding
> > > patches are for refactoring, but patch "vfio-user: Do not delete the
> > > subregion" does fix use-after-finalization.
> > > 
> > > * https://lore.kernel.org/qemu-devel/20250901-mr-v1-0-
> > > dd7cb6b1480b@rsg.ci.i.u-tokyo.ac.jp/
> > > 
> > > Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> > 
> > This makes sense, but I think it is not bisectable, because of this in
> > memory_region_del_subregion():
> > 
> >      assert(subregion->container == mr);
> >      subregion->container = NULL;
> > 
> > You would need to add a temporary
> > 
> >      if (subregion->container == NULL) {
> >          return;
> >      }
> > 
> > and undo it at the end of the series.  Do you agree?  With this change I
> > can apply it.
> 
> It is unnecessary because patch "qdev: Automatically delete memory
> subregions" satisfies the following:
> 
> 1. the device-specific code can assume that subregions they added are
> present until it finishes unrealization. The unrealize() callback can also
> assume the subregions are present and delete them. qdev satisfies this by
> deleting subregions only after calling the unrealize().
> 
> 2. qdev should delete the remaining subregions before it finishes
> unrealization to ensure that the devices are hidden from the guest. qdev
> satisfies this by checking if memory regions have containers before
> deleting.

There's another option, which is to have the auto-detach patch for qdev
merged, meanwhile we can still keep the "good citizens" who do explicit
detachments in unrealize(), so that it won't confuse the reader on a
specific device about when did the MR went away.  I believe there will be
developers get confused by them otherwise.

The auto detach will only be a final safety belt to make sure MRs won't get
leaked.

If that is an option, it may also explain my original question on what
problem we're fixing... the extreme case is what this series removed
afterwards are exactly all such uses that may need an explicit detachment
of MRs (ignoring there can be owner-shared MR links, which is even out of
the picture).  Then the auto-detach feature may fix nothing, so we merge a
patch, looks good, logically sensible, but not functioning.

The reality might be, we may still have some qdev that should unlink the
MRs in unrealize(), but they didn't.  However again I suspect it's rare,
especially considering the recently merged auto-detach in finalize().  I'd
just go and fix them, but only if they're a real problem (aka,
unpluggable). Then, we know why we introduce a change, and especially, when
the change can go away.

OTOH, if we merge auto-detach of qdev MRs, we likely need to stick it
forever because justifying it not needed will be extremely hard if not
impossible.. after all we do not know what it fixes.  So if we can at least
list the devices that may be fixed with it in the commit message, it would
be a benefit.

Once again, feel free to treat my comment as a pure question.

Thanks,

-- 
Peter Xu