Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Missing cleanup in `reserve_module_tags()` error path leaves dangling pointer in maple tree. - [High] `release_module_tags()` uses `mas_store()` which can fail silently under memory pressure, leaving a dangling pointer. -- --- Patch [1]: [PATCH v5 1/2] alloc_tag: move release_module_tags() above reserve_module_tags() --- commit 1dcbb4a041b40aefa702fd60db8fbd78ff223508 Author: Hao Ge <[email protected]> alloc_tag: move release_module_tags() above reserve_module_tags() This patch moves the release_module_tags() cleanup helper above reserve_module_tags() to avoid the need for a forward declaration. > diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c > index b334103104779..2070e682fe106 100644 > --- a/mm/alloc_tag.c > +++ b/mm/alloc_tag.c [ ... ] > @@ -835,6 +835,52 @@ static int vm_module_tags_populate(void) > return 0; > } > > +static void release_module_tags(struct module *mod, bool used) > +{ [ ... ] > +release_area: > + mas_store(&mas, used ? &unloaded_mod : NULL); > + val = mas_prev_range(&mas, 0); > + if (val == &prepend_mod) > + mas_store(&mas, NULL); [Severity: High] This is a pre-existing issue, but since the code is being moved here: could mas_store() fail silently under memory pressure, leaving a dangling pointer? When unloading a module, release_module_tags() calls mas_store() to clear the module's reservation. If the internal maple tree allocation fails, is the error state ignored? This could leave the tree retaining the old pointer, which might lead to a use-after-free when the module subsystem frees it. Would using mas_store_gfp(..., GFP_KERNEL) be needed here to ensure the maple tree can sleep to allocate memory? > +out: > + mas_unlock(&mas); > +} > + > static void *reserve_module_tags(struct module *mod, unsigned long size, > unsigned int prepend, unsigned long align) > { [Severity: High] This isn't a bug introduced by this patch, but the commit message mentions that reserve_module_tags() can fail after storing the reservation in the maple tree and should call release_module_tags() to undo it. However, looking at the existing reserve_module_tags() code down the line, if vm_module_tags_populate() fails: reserve_module_tags() { ... grow_res = vm_module_tags_populate(); if (grow_res) { shutdown_mem_profiling(true); pr_err("Failed to allocate memory...\n", mod->name); return ERR_PTR(grow_res); } ... } Does this error path still return without actually calling release_module_tags(mod, false)? If the undo call is still missing here, it appears the maple tree mod_area_mt might retain a pointer to the module that is being freed, potentially leading to a use-after-free during subsequent lookups. Should that call be added here in a follow-up patch? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
