[PATCH] memory: Make FlatView root references weak

Akihiko Odaki posted 1 patch 2 hours ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20251027-root-v1-1-ddf92b9058be@rsg.ci.i.u-tokyo.ac.jp
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
system/memory.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
[PATCH] memory: Make FlatView root references weak
Posted by Akihiko Odaki 2 hours ago
docs/devel/memory.rst says "memory_region_ref and memory_region_unref
are never called on aliases", but these functions are called for
FlatView roots, which can be aliases.

This causes object overwrite hazard in pci-bridge. Specifically,
pci_bridge_region_init() expects that there are no references to
w->alias_io after object_unparent() is called, allowing it to reuse the
associated storage. However, if a parent bus still holds a reference to
the existing object as a FlatView's root, the storage is still in use,
leading to an overwrite. This hazard can be confirmed by adding the
following code to pci_bridge_region_init():

PCIDevice *parent_dev = parent->parent_dev;
assert(!object_dynamic_cast(OBJECT(parent_dev), TYPE_PCI_BRIDGE) ||
       PCI_BRIDGE(parent_dev)->as_io.current_map->root != &w->alias_io);

This assertion fails when running:
meson test -C build qtest-x86_64/bios-tables-test \
    '--test-args=-p /x86_64/acpi/piix4/pci-hotplug/no_root_hotplug'

Make the references of FlatView roots "weak" (i.e., remove the
reference to a root automatically removed when it is finalized) to
avoid calling memory_region_ref and memory_region_unref and fix the
hazard with pci-bridge.

Alternative solutions (like removing the "never called on aliases"
statement or detailing the exception) were rejected because the alias
invariant is still relied upon in several parts of the codebase, and
updating existing code to align with a new condition is non-trivial.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 system/memory.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 8b84661ae36c..08fe5e791224 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -266,7 +266,6 @@ static FlatView *flatview_new(MemoryRegion *mr_root)
     view = g_new0(FlatView, 1);
     view->ref = 1;
     view->root = mr_root;
-    memory_region_ref(mr_root);
     trace_flatview_new(view, mr_root);
 
     return view;
@@ -301,7 +300,6 @@ static void flatview_destroy(FlatView *view)
         memory_region_unref(view->ranges[i].mr);
     }
     g_free(view->ranges);
-    memory_region_unref(view->root);
     g_free(view);
 }
 
@@ -1796,6 +1794,12 @@ void memory_region_init_iommu(void *_iommu_mr,
 static void memory_region_finalize(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
+    gpointer key;
+    gpointer value;
+
+    if (g_hash_table_steal_extended(flat_views, mr, &key, &value)) {
+        ((FlatView *)value)->root = NULL;
+    }
 
     /*
      * Each memory region (that can be freed) must have an owner, and it

---
base-commit: 36076d24f04ea9dc3357c0fbe7bb14917375819c
change-id: 20251024-root-d431450fcbbf

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