On 2/13/26 22:48, Joelle van Dyne wrote: > 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 for the clarification. Curious which gfx mode showed this problem, virgl or venus? What's the GPU card/model on host? -- Best regards, Dmitry
