Re: [PATCH v5 2/2] alloc_tag: fix undetected compressed tag overflow when profiling is disabled

[Hao Ge <hao.ge@linux.dev>]

On 2026/8/12 14:32, Hao Ge wrote:
> CC Andrew and Suren
> 
> 
> On 2026/8/12 14:04, sashiko-bot@kernel.org wrote:
>> 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 <hao.ge@linux.dev>
>>
>> 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.
> 
> The per-CPU counter leak is pre-existing, not introduced by the retry
> 
>                                                      mechanism.
> 
> The root cause is that shutdown_mem_profiling() permanently sets
> 
> 
> mem_profiling_support to false, after which needs_section_mem()
> 
>                                                        returns false for every subsequent module. Their codetag sections are
> 
>                                         placed as regular data, so they are never inserted into the alloc_tag
> 
>                       maple tree. Yet codetag_load_module() still unconditionally invokes
> 
>          the load_module() callback, which allocates percpu counters that
> 
> release_module_tags() cannot find on unload.
> 
> I'll send out a separate patch to fix this leak later.
> 
> 

Sorry for the messy indentation — it's down to my mail client's formatting config.
A colleague pointed out the official recommended indent configs for kernel mail clients,
which I'd missed entirely.
https://elixir.bootlin.com/linux/v7.2-rc7/source/Documentation/process/email-clients.rst
Sorry for all the hassle I've caused everyone earlier.
The patch fixing the per-CPU counter leak has been sent out:
https://lore.kernel.org/all/20260813093421.135230-2-hao.ge@linux.dev/

Thanks
Best Regards
Hao

> 
>>
>>> +    }
>>>         /* 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.
>>