On 2/4/26 22:04, Michael S. Tsirkin 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); > memory_region_add_subregion(&b->hostmem, offset, mr); > memory_region_set_enabled(mr, true); > > @@ -186,7 +186,7 @@ virtio_gpu_virgl_unmap_resource_blob(VirtIOGPU *g, > /* memory region owns self res->mr object and frees it by itself */ > memory_region_set_enabled(mr, false); > memory_region_del_subregion(&b->hostmem, mr); > - object_unparent(OBJECT(mr)); > + object_unref(OBJECT(mr)); > } > > return 0;
Hello Michael, This patch introduces regression. Running any venus application results in a crash: Thread 2 "qemu-system-x86" received signal SIGSEGV, Segmentation fault. (gdb) bt #0 0x00007ffff56565e2 in __strcmp_evex () at /lib64/libc.so.6 #1 0x0000555555841bdb in find_fd (head=0x5555572337d0 <cpr_state>, name=0x0, id=0) at ../migration/cpr.c:68 #2 cpr_delete_fd (name=name@entry=0x0, id=id@entry=0) at ../migration/cpr.c:77 #3 0x000055555582290a in qemu_ram_free (block=0x7ff7e93aa7f0) at ../system/physmem.c:2615 #4 0x000055555581ae02 in memory_region_finalize (obj=<optimized out>) at ../system/memory.c:1816 #5 0x0000555555a70ab9 in object_deinit (obj=<optimized out>, type=<optimized out>) at ../qom/object.c:715 #6 object_finalize (data=0x7ff7e936eff0) at ../qom/object.c:729 #7 object_unref (objptr=0x7ff7e936eff0) at ../qom/object.c:1232 #8 0x0000555555814fae in memory_region_unref (mr=<optimized out>) at ../system/memory.c:1848 #9 flatview_destroy (view=0x555559ed6c40) at ../system/memory.c:301 #10 0x0000555555bfc122 in call_rcu_thread (opaque=<optimized out>) at ../util/rcu.c:324 #11 0x0000555555bf17a7 in qemu_thread_start (args=0x555557b99520) at ../util/qemu-thread-posix.c:393 #12 0x00007ffff556f464 in start_thread () at /lib64/libc.so.6 #13 0x00007ffff55f25ac in __clone3 () at /lib64/libc.so.6 There is a v2 version of this patch that doesn't crash [1]. Was v1 applied by mistake instead of v2? [1] https://lore.kernel.org/qemu-devel/[email protected]/ -- Best regards, Dmitry
