Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Per-CPU memory leak of module allocation tag counters on module unload when falling back to regular section allocation. - [Medium] Skipping memcpy for SHT_NOBITS codetag sections leaves them filled with uninitialized kernel memory. Pre-existing issues: - [High] AB-BA deadlock between `/proc/allocinfo` file readers (`pde->in_use`) and module loading (`cttype->mod_lock`). -- commit 073ca6b0680c884f9d863cdcbd7d5db665a17c3b Author: Hao Ge <[email protected]> module: allocate codetag sections before the regular module layout Whether a codetag section goes to the codetag region is decided by layout_sections() and asked again in move_module(). A concurrent load can shut profiling down in between, and move_module() then copies the section to offset 0 of its regular destination, overwriting whatever is there. Link: https://lore.kernel.org/all/[email protected]/ > diff --git a/kernel/module/main.c b/kernel/module/main.c > index b114b815187a9..b94830c5b9a10 100644 > --- a/kernel/module/main.c > +++ b/kernel/module/main.c [ ... ] > @@ -2940,6 +2901,47 @@ static bool blacklisted(const char *module_name) > } > core_param(module_blacklist, module_blacklist, charp, 0400); > > +/* > + * Allocate codetag sections separately. They are loaded into preallocated > + * contiguous memory because they may still be used after the module is > + * unloaded. > + * > + * If the separate allocation overflows, allocate the section normally > + * so that the module can still be loaded. > + */ > +static int allocate_codetag_sections(struct load_info *info) > +{ > + for (unsigned int i = 1; i < info->hdr->e_shnum; i++) { > + Elf_Shdr *shdr = &info->sechdrs[i]; > + const char *sname = info->secstrings + shdr->sh_name; > + void *dest; > + > + if (!codetag_needs_module_section(info->mod, sname, > shdr->sh_size)) > + continue; > + > + dest = codetag_alloc_module_section(info->mod, sname, > shdr->sh_size, > + arch_mod_section_prepend(info->mod, i), > shdr->sh_addralign); > + if (WARN_ON(!dest)) { > + codetag_free_module_sections(info->mod); > + return -EINVAL; > + } > + if (dest == ERR_PTR(-EAGAIN)) > + /* Allocate the section as a regular section. */ > + continue; [Severity: High] Does this code leak the per-cpu memory on module unload when falling back to regular section allocation? If tags_addressable() returns false, reserve_module_tags() returns -EAGAIN and removes the module from mod_area_mt. The fallback mechanism here allocates the tags in regular module memory, but the module loading process still unconditionally allocates per-cpu counters for each tag, oblivious to the fallback or the disabled profiling state: for (tag = start_tag; tag < stop_tag; tag++) { WARN_ON(tag->counters); tag->counters = alloc_percpu(struct alloc_tag_counters); On unload, release_module_tags() fails to find the module in mod_area_mt because it was removed, skipping clean_unused_counters() and leaking the previously allocated per-cpu memory. > + if (IS_ERR(dest)) { > + codetag_free_module_sections(info->mod); > + return PTR_ERR(dest); > + } > + > + if (shdr->sh_type != SHT_NOBITS) > + memcpy(dest, (void *)shdr->sh_addr, shdr->sh_size); [Severity: Medium] Could this expose uninitialized kernel memory if the codetag section is parsed from an ELF module with the SHT_NOBITS type? The code skips memcpy() but fails to use memset() to zero-initialize the destination buffer. Regular module sections are safe because module_memory_alloc() explicitly zeroes them, but reserve_module_tags() backs its memory with alloc_pages_bulk_node() without the __GFP_ZERO flag: mm/alloc_tag.c:vm_module_tags_populate() { ... allocated = alloc_pages_bulk_node(GFP_KERNEL | __GFP_NOWARN, NUMA_NO_NODE, more_pages - nr, next_page + nr); ... } > + shdr->sh_addr = (unsigned long)dest; > + shdr->sh_entsize = SH_ENTSIZE_STANDALONE; > + } > + > + return 0; > +} > + [ ... ] > diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c > index 2070e682fe106..112a014d4b890 100644 > --- 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; > - if (mem_alloc_profiling_enabled() && !tags_addressable()) { > + if (!tags_addressable()) { > shutdown_mem_profiling(true); [Severity: High] This is a pre-existing issue, but does this shutdown_mem_profiling(true) call introduce an AB-BA deadlock between /proc/allocinfo file readers (pde->in_use) and module loading (cttype->mod_lock)? A user reading /proc/allocinfo acquires the procfs in_use reference in use_pde(), then blocks on down_read(&alloc_tag_cttype->mod_lock) in allocinfo_start(). Concurrently, if a module loads and tags_addressable() fails, codetag_alloc_module_section() holds down_write(&cttype->mod_lock) and calls shutdown_mem_profiling(true), which removes the proc entry. This removal waits indefinitely for the in_use reference to drop to zero, hanging both procfs reads and module loading. > - 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(); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
