On Fri, Feb 13, 2026 at 8:18 AM Alex Bennée <[email protected]> wrote: > > Peter Maydell <[email protected]> writes: > > > On Wed, 4 Feb 2026 at 19:04, Michael S. Tsirkin <[email protected]> wrote: > >> > >> From: Joelle van Dyne <[email protected]> > >> > >> When `owner` == `mr`, `object_unparent` will crash: > >> > >> object_unparent(mr) -> > >> object_property_del_child(mr, mr) -> > >> object_finalize_child_property(mr, name, mr) -> > >> object_unref(mr) -> > >> object_finalize(mr) -> > >> object_property_del_all(mr) -> > >> object_finalize_child_property(mr, name, mr) -> > >> object_unref(mr) -> > >> fail on g_assert(obj->ref > 0) > >> > >> However, passing a different `owner` to `memory_region_init` does not > >> work. `memory_region_ref` has an optimization where it takes a ref > >> only on the owner. That means when flatviews are created, it does not > >> take a ref on the region and you can get a UAF from `flatview_destroy` > >> called from RCU. > >> > >> The correct fix therefore is to use `NULL` as the name which will set > >> the `owner` but not the `parent` (which is still NULL). This allows us > >> to use `memory_region_ref` on itself while not having to rely on unparent > >> for cleanup. > >> > >> Signed-off-by: Joelle van Dyne <[email protected]> > >> Reviewed-by: Akihiko Odaki <[email protected]> > >> Reviewed-by: Michael S. Tsirkin <[email protected]> > >> Signed-off-by: Michael S. Tsirkin <[email protected]> > >> Message-Id: <[email protected]> > >> --- > >> hw/display/virtio-gpu-virgl.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c > >> index 07f6355ad6..6a83fb63c8 100644 > >> --- a/hw/display/virtio-gpu-virgl.c > >> +++ b/hw/display/virtio-gpu-virgl.c > >> @@ -120,7 +120,7 @@ virtio_gpu_virgl_map_resource_blob(VirtIOGPU *g, > >> vmr->g = g; > >> > >> mr = &vmr->mr; > >> - memory_region_init_ram_ptr(mr, OBJECT(mr), "blob", size, data); > >> + memory_region_init_ram_ptr(mr, OBJECT(mr), NULL, size, data); > > > > This looks very odd. The owner of an MR should be the > > device that that MR belongs to, not the MR itself, > > and usually not NULL either. The name should be something > > useful for people looking at the HMP info output about > > memory layouts. The owner of the MR was set to itself before the change and this caused a crash on my ARM64 host (I'm not sure why this wasn't a issue beforehand on other architectures, I suspect it was a race condition that was always won before). The original fix (see v2) was to call memory_region_init_ram_ptr() with "owner" set to VirtIOGPU *g and then manually setting "mr->owner = OBJECT(mr)" after the call, thus breaking the self <-> parent loop that caused the original crash. However, this was brittle as we were modifiying MR internals outside of MR APIs so the suggestion was made to use NULL for "name" and "object_unref" instead of "object_unparent" in cleanup. This MR was properly an orphan and does not have a parent. We depended on the behavior of "memory_region_do_init" to not call "object_property_add_child" when "name" is NULL. The APIs were not clear if "name" is allowed to be NULL but the existance of that code path led to the assumption that NULL was permitted as an argument. Obivously this assumption was flawed as the crash log that Dmitry shared showed that cpr_name() in physmem.c will return NULL and this causes the NULL name to be passed to cpr_delete_fd(). There might be other places that expect the name to be non-NULL as well.
> > > > If there's a use-after-free issue then I suspect that the right > > fix must be somewhere else, not here. The obivous fix is to revert the change but the original crash referenced in the patch will still be an issue. Alternatively, it seems that the v2 version is unclean but can still work because we just need the MR to not have itself as the parent. This would be the minimal patch that addresses both issues. (A proper fix should also remove the code path in memory_region_do_init() for when name is NULL and instead return an error because that path is invalid.) Another alternative would be to modify the MR APIs with a clear way to create an orphan MR. Finally, object_unparent() could recognize when the object is its own parent and handle that case gracefully. Let me know what people's preferred solution is and I'll whip up a patch. > > The blobs really cause issues for our MemoryRegion code because they are > transient and aren't easily cleaned up with RCU because of the dance we > have to do between qemu and virglrenderer for the underlying memory. > There have been multiple attempts to clean this up and so far I don't > think we've managed to reach a solid solution. > > > > > thanks > > -- PMM > > -- > Alex Bennée > Virtualisation Tech Lead @ Linaro
