Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] Unconditional allocation of per-CPU counters for modules causes a memory leak when memory profiling is disabled and the module load is retried. Pre-existing issues: - [Critical] Concurrent module loading can cause an out-of-bounds memory write and module corruption because the global profiling flag is toggled without cross-phase synchronization. - [High] A race condition on the global module tags size variable can lead to unmapped virtual memory access and a kernel panic. - [High] If page population fails, a dangling module pointer is left in the maple tree, leading to a Use-After-Free. -- --- Patch [2]: [PATCH v5 2/2] alloc_tag: fix undetected compressed tag overflow when profiling is disabled --- commit ffb2b208763f32cdfb3d6cb740d4d4efa5c99ae0 Author: Hao Ge <[email protected]> alloc_tag: fix undetected compressed tag overflow when profiling is disabled This patch modifies module layout and allocation to retry loading without memory profiling if the compressed tag limit is exceeded. It removes the profiling enabled guard when checking for tag overflow and handles the fallback by resetting section flags and repeating the layout process. > diff --git a/kernel/module/main.c b/kernel/module/main.c > --- a/kernel/module/main.c > +++ b/kernel/module/main.c [ ... ] > @@ -2966,18 +2967,38 @@ static struct module *layout_and_allocate(struct > load_info *info, int flags) > */ > module_mark_ro_after_init(info->hdr, info->sechdrs, info->secstrings); > > + /* > + * Save the sizes reserved by module_frob_arch_sections() so they can > + * be restored if we retry below. > + */ > + for_each_mod_mem_type(type) > + frob_size[type] = info->mod->mem[type].size; > + > /* > * Determine total sizes, and put offsets in sh_entsize. For now > * this is done generically; there doesn't appear to be any > * special cases for the architectures. > */ > +retry: > layout_sections(info->mod, info); > layout_symtab(info->mod, info); > > /* Allocate and move to the final place */ > err = move_module(info->mod, info); [Severity: Critical] This isn't a bug introduced by this patch, but can concurrent module loading cause memory corruption if profiling is disabled mid-flight? In move_module(), if one thread disables profiling globally via shutdown_mem_profiling() while another thread is halfway through module loading (between layout calculation and memory movement), the second thread's section offset queries could evaluate to false. This would cause the module loader to use an un-incremented 0 offset and incorrectly write the codetag section into the base memory, overwriting the start of the module's core memory block. > - if (err) > - return ERR_PTR(err); > + if (err) { > + if (err != -EAGAIN) > + return ERR_PTR(err); > + /* > + * -EAGAIN means profiling was disabled but the module > + * can still load without it. Reset state and retry. > + */ > + rewrite_section_headers(info, flags); > + for_each_mod_mem_type(type) > + info->mod->mem[type].size = frob_size[type]; > + info->sechdrs[info->index.sym].sh_flags &= ~(unsigned > long)SHF_ALLOC; > + info->sechdrs[info->index.str].sh_flags &= ~(unsigned > long)SHF_ALLOC; > + goto retry; [Severity: High] Does this retry mechanism cause a memory leak for per-CPU counters? When the module load is retried via this new loop, the codetag section is treated as standard memory and bypasses maple tree registration. However, the subsequent codetag module initialization unconditionally allocates per-CPU memory for these tags regardless of the profiling state. During module unload in release_module_tags(), the cleanup path exits early because the module cannot be found in the maple tree, permanently leaking the per-CPU memory on every unload. > + } > > /* Module has been copied to its final place now: return it. */ > mod = (void *)info->sechdrs[info->index.mod].sh_addr; > diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c > --- a/mm/alloc_tag.c > +++ b/mm/alloc_tag.c [ ... ] > @@ -950,10 +950,12 @@ static void *reserve_module_tags(struct module *mod, > unsigned long size, > int grow_res; > > module_tags.size = offset + size; [Severity: High] This is a pre-existing issue, but does the update to module_tags.size race with concurrent module loads? The high-water mark for the module tags size is updated here without a lock after the maple tree lock is released. If two concurrent module loads race on this update, a higher offset might be overwritten by a smaller one. Subsequently, vm_module_tags_populate() relies on this corrupted size and would fail to map the physical pages for the higher virtual addresses, leading to a page fault when the module accesses its allocated tags. > - if (mem_alloc_profiling_enabled() && !tags_addressable()) { > + if (!tags_addressable()) { > shutdown_mem_profiling(true); > - pr_warn("With module %s there are too many tags to fit > in %d page flag bits. Memory allocation profiling is disabled!\n", > - mod->name, NR_UNUSED_PAGEFLAG_BITS); > + pr_warn_once("With module %s there are too many tags to > fit in %d page flag bits. Memory allocation profiling is disabled!\n", > + mod->name, NR_UNUSED_PAGEFLAG_BITS); > + release_module_tags(mod, false); > + return ERR_PTR(-EAGAIN); > } > > grow_res = vm_module_tags_populate(); [Severity: High] This is a pre-existing issue, but could this lead to a dangling pointer in the maple tree if page population fails? If vm_module_tags_populate() fails and returns an error, the allocation routine returns an error here but doesn't remove the newly inserted module pointer from the maple tree. Since this error bypasses the standard codetag cleanup routines in the module loader (where codetag_section_found might remain false in move_module()), the maple tree retains a pointer to the aborted module. Once the module structure is freed, future maple tree iterations could dereference this freed pointer. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
