Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] mm: crash on freeing static percpu variable when module load fails or profiling disabled --
commit 1ed77a1b45b5a7a434417c336e13dff08f4838d1 Author: Hao Ge <[email protected]> alloc_tag: skip percpu counter allocation when profiling is disabled After shutdown_mem_profiling() clears mem_profiling_support, needs_section_mem() returns false, so later modules have their codetag section placed as regular data and never enter the alloc_tag maple tree. codetag_load_module() still called load_module(), which allocated a percpu counter for every tag; release_module_tags() could not find these modules on unload, so the counters leaked. > diff --git a/lib/codetag.c b/lib/codetag.c > index a9cda4c962a30..a0b600720afc1 100644 > --- a/lib/codetag.c > +++ b/lib/codetag.c [ ... ] > @@ -388,7 +390,11 @@ void codetag_unload_module(struct module *mod) > ++cttype->content_id; > } > up_write(&cttype->mod_lock); > - if (found && cttype->desc.free_section_mem) > + /* > + * A module whose module_load() returned -EOPNOTSUPP is not > + * in the idr but may still hold reserved section memory. > + */ > + if (cttype->desc.free_section_mem) > cttype->desc.free_section_mem(mod, true); > } > mutex_unlock(&codetag_lock); [Severity: High] Does this unconditional call to free_section_mem() lead to a crash when freeing statically allocated percpu memory? On architectures defining CONFIG_ARCH_MODULE_NEEDS_WEAK_PER_CPU, DEFINE_ALLOC_TAG initializes tag->counters to the statically allocated &_shared_alloc_tag. If a module load skips dynamic counter allocation (either because mem_profiling_support is false and load_module() returns -EOPNOTSUPP, or if alloc_percpu() fails with -ENOMEM), tag->counters remains pointing to &_shared_alloc_tag. Because free_section_mem() is now called unconditionally here on module unload or load failure, it triggers the cleanup path for these uninitialized tags: release_module_tags() -> clean_unused_counters() In clean_unused_counters(), since the static _shared_alloc_tag has 0 bytes, it unconditionally calls free_percpu(tag->counters): mm/alloc_tag.c:clean_unused_counters() { ... if (!counter.bytes) { free_percpu(tag->counters); tag->counters = NULL; ... } Calling free_percpu() on a statically allocated per-cpu variable that doesn't belong to a managed chunk can corrupt memory and cause a crash. > diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c > index 1a6108a596684..1106fc60dc89c 100644 > --- a/mm/alloc_tag.c > +++ b/mm/alloc_tag.c > @@ -981,6 +981,10 @@ static int load_module(struct module *mod, struct > codetag *start, struct codetag > struct alloc_tag *stop_tag; > struct alloc_tag *tag; > > + /* Profiling disabled: load the module without its tags. */ > + if (!mem_profiling_support) > + return -EOPNOTSUPP; > + > /* percpu counters for core allocations are already statically > allocated */ > if (!mod) > return 0; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
