Make IgvmMemoryRegion a QOM object, embedding MemoryRegion, and use it as the region owner. The IGVM list and memory subsystem can now hold references independently, and QOM frees after the last reference is dropped, fixing leaks reported by ASAN while running tests.
- object_unparent() is no longer needed, children are automatically unref - switch to memory_region_init_ram_flags_nomigrate() as other helpers expect a DEVICE Signed-off-by: Marc-André Lureau <[email protected]> --- backends/igvm.c | 97 +++++++++++++++++++++++++----------------- include/system/igvm-internal.h | 9 ++-- 2 files changed, 64 insertions(+), 42 deletions(-) diff --git a/backends/igvm.c b/backends/igvm.c index 784a56504e77..6cd192f2f5ff 100644 --- a/backends/igvm.c +++ b/backends/igvm.c @@ -35,6 +35,12 @@ #define IGVM_VHT_OPTIONAL_BIT (1U << 31) #endif +struct IgvmMemoryRegion { + Object parent_obj; + MemoryRegion mr; + QTAILQ_ENTRY(IgvmMemoryRegion) next; +}; + /* * Bit 31 of the variable header type indicates that the header is * optional and can be safely ignored by a loader that does not @@ -221,10 +227,13 @@ static void *qigvm_prepare_memory(QIgvm *ctx, uint64_t addr, uint64_t size, int region_identifier, Error **errp) { ERRP_GUARD(); - IgvmMemoryRegion *imr = NULL; + g_autoptr(IgvmMemoryRegion) imr = NULL; Int128 gpa_region_size; + g_autofree char *region_name = NULL; + uint32_t flags = 0; MemoryRegionSection mrs = memory_region_find(get_system_memory(), addr, size); + if (mrs.mr) { if (!memory_region_is_ram(mrs.mr)) { memory_region_unref(mrs.mr); @@ -247,36 +256,29 @@ static void *qigvm_prepare_memory(QIgvm *ctx, uint64_t addr, uint64_t size, return NULL; } return qemu_map_ram_ptr(mrs.mr->ram_block, mrs.offset_within_region); - } else { - /* - * The region_identifier is the is the index of the IGVM directive that - * contains the page with the lowest GPA in the region. This will - * generate a unique region name. - */ - g_autofree char *region_name = - g_strdup_printf("igvm.%X", region_identifier); - imr = g_new0(IgvmMemoryRegion, 1); - imr->mr = g_new0(MemoryRegion, 1); - if (ctx->machine_state->cgs && - ctx->machine_state->cgs->require_guest_memfd) { - if (!memory_region_init_ram_guest_memfd(imr->mr, NULL, - region_name, size, errp)) { - g_free(imr->mr); - g_free(imr); - return NULL; - } - } else { - if (!memory_region_init_ram(imr->mr, NULL, region_name, size, - errp)) { - g_free(imr->mr); - g_free(imr); - return NULL; - } - } - memory_region_add_subregion(get_system_memory(), addr, imr->mr); - QTAILQ_INSERT_TAIL(&ctx->cfg->memory_regions, imr, next); - return memory_region_get_ram_ptr(imr->mr); } + + /* + * The region_identifier is the is the index of the IGVM directive that + * contains the page with the lowest GPA in the region. This will + * generate a unique region name. + */ + region_name = g_strdup_printf("igvm.%X", region_identifier); + imr = IGVM_MEMORY_REGION(object_new(TYPE_IGVM_MEMORY_REGION)); + if (ctx->machine_state->cgs && + ctx->machine_state->cgs->require_guest_memfd) { + flags = RAM_GUEST_MEMFD; + } + if (!memory_region_init_ram_flags_nomigrate(&imr->mr, OBJECT(imr), + region_name, size, + flags, errp)) { + return NULL; + } + vmstate_register_ram_global(&imr->mr); + memory_region_add_subregion(get_system_memory(), addr, &imr->mr); + object_ref(imr); /* for the list */ + QTAILQ_INSERT_TAIL(&ctx->cfg->memory_regions, imr, next); + return memory_region_get_ram_ptr(&imr->mr); } static int qigvm_type_to_cgs_type(IgvmPageDataType memory_type, bool unmeasured, @@ -1118,14 +1120,33 @@ void qigvm_cleanup_memory(IgvmCfg *cfg) { IgvmMemoryRegion *imr, *tmp; - QTAILQ_FOREACH_SAFE(imr, &cfg->memory_regions, next, tmp) - { - trace_qigvm_cleanup_memory(imr->mr->name); - memory_region_del_subregion(get_system_memory(), imr->mr); - vmstate_unregister_ram(imr->mr, NULL); + QTAILQ_FOREACH_SAFE(imr, &cfg->memory_regions, next, tmp) { + trace_qigvm_cleanup_memory(imr->mr.name); QTAILQ_REMOVE(&cfg->memory_regions, imr, next); - /* this triggers MemoryRegion cleanup */ - object_unparent(OBJECT(imr->mr)); - g_free(imr); + object_unref(OBJECT(imr)); } } + +static void qigvm_memory_region_finalize(Object *obj) +{ + IgvmMemoryRegion *imr = IGVM_MEMORY_REGION(obj); + + if (imr->mr.container) { + memory_region_del_subregion(imr->mr.container, &imr->mr); + } + vmstate_unregister_ram(&imr->mr, NULL); +} + +static const TypeInfo qigvm_memory_region_info = { + .name = TYPE_IGVM_MEMORY_REGION, + .parent = TYPE_OBJECT, + .instance_size = sizeof(IgvmMemoryRegion), + .instance_finalize = qigvm_memory_region_finalize, +}; + +static void qigvm_register_types(void) +{ + type_register_static(&qigvm_memory_region_info); +} + +type_init(qigvm_register_types); diff --git a/include/system/igvm-internal.h b/include/system/igvm-internal.h index 9e9fa1d9afdb..625d2f9a9f83 100644 --- a/include/system/igvm-internal.h +++ b/include/system/igvm-internal.h @@ -18,10 +18,11 @@ #include "system/confidential-guest-support.h" #include <igvm/igvm.h> -typedef struct IgvmMemoryRegion { - QTAILQ_ENTRY(IgvmMemoryRegion) next; - MemoryRegion *mr; -} IgvmMemoryRegion; +typedef struct IgvmMemoryRegion IgvmMemoryRegion; + +#define TYPE_IGVM_MEMORY_REGION "igvm-memory-region" + +OBJECT_DECLARE_SIMPLE_TYPE(IgvmMemoryRegion, IGVM_MEMORY_REGION) struct IgvmCfg { Object parent_obj; -- 2.55.0.543.g5ebe2ebe4ea8
