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

On 2026/8/16 23:16, Suren Baghdasaryan wrote:
> On Sat, Aug 15, 2026 at 3:45 AM Petr Pavlu <[email protected]> wrote:
>>
>> On 8/12/26 7:41 AM, Hao Ge wrote:
>>> In reserve_module_tags(), the tag overflow check is gated on
>>> mem_alloc_profiling_enabled():
>>>
>>>     if (mem_alloc_profiling_enabled() && !tags_addressable())
>>>
>>> If profiling is toggled off at runtime and a module is loaded whose
>>> tags exceed the compressed-mode limit, shutdown_mem_profiling() is
>>> skipped. vm_module_tags_populate() still maps memory for the tags and
>>> the module loads successfully, but the total tag count now exceeds what
>>> NR_UNUSED_PAGEFLAG_BITS can address.
>>>
>>> Once profiling is re-enabled, ref_to_idx() computes each tag's index
>>> as its position in the alloc_tag array. update_page_tag_ref() masks
>>> it to alloc_tag_ref_mask before storing in page->flags. Indices
>>> beyond the mask are truncated and idx_to_ref() resolves them to wrong
>>> tags.
>>>
>>> This silently corrupts /proc/allocinfo: allocated pages get attributed
>>> to the wrong call sites, so the statistics it reports are wrong.
>>>
>>> mem_alloc_profiling_enabled() and mem_profiling_compressed are
>>> independent. Once compressed mode is established at boot, it stays
>>> active regardless of runtime toggles of mem_profiling.
>>>
>>> Remove the mem_alloc_profiling_enabled() guard. On overflow, shut down
>>> profiling, release the reservation, and return -EAGAIN so that
>>> layout_and_allocate() retries with profiling disabled: codetag sections
>>> are then placed as regular module data and the module loads without
>>> profiling rather than being rejected entirely.
>>
>> When the described overflow occurs, why should codetag sections be
>> placed as regular module data? Will the codetag support use them in any
>> way, or do they simply waste space? Is the issue that alloc_hooks()
>> creates relocations pointing into .codetag.alloc_tags?
> 
> Correct, alloc_hooks() will have references into .codetag.alloc_tags.
> With mem_profiling_support=false they should technically never be used
> but I don't think it's a good idea to skip .codetag.alloc_tags section
> allocation and to leave dangling pointers. Also the case described
> here is an outlier, so optimizing it would not yield much benefit.
> 
>>
>>>
>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
>>> Cc: [email protected]
>>> Suggested-by: Suren Baghdasaryan <[email protected]>
>>> Signed-off-by: Hao Ge <[email protected]>
>>> ---
>>>  kernel/module/main.c | 25 +++++++++++++++++++++++--
>>>  mm/alloc_tag.c       |  8 +++++---
>>>  2 files changed, 28 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>> index 46dd8d25a605..ed26f167be84 100644
>>> --- a/kernel/module/main.c
>>> +++ b/kernel/module/main.c
>>> @@ -2944,6 +2944,7 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>>  {
>>>       struct module *mod;
>>>       int err;
>>> +     unsigned long frob_size[MOD_MEM_NUM_TYPES];
>>
>> frob_size is used to store values of module_memory::size, which has type
>> `unsigned int`. The types should match.
>>
>>>
>>>       /* Allow arches to frob section contents and sizes.  */
>>>       err = module_frob_arch_sections(info->hdr, info->sechdrs,
>>> @@ -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);
>>> -     if (err)
>>> -             return ERR_PTR(err);
>>> +     if (err) {
>>> +             if (err != -EAGAIN)
>>> +                     return ERR_PTR(err);
>>
>> The move_module() logic is non-trivial. -EAGAIN could be returned by
>> other code, now or in the future.
> 
> That's a good point.
> 
>>
>>> +             /*
>>> +              * -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;
>>
>> Why is it necessary to reset SHF_ALLOC for .symtab and .strtab here?
> 
> I believe layout_symtab() sets that bit and to retry we need to reset
> it. But I might be wrong here.
> 

Thanks Suren.

And yes, IMHO layout_symtab() is the reason. In the module ELF, .symtab and.strtab
carry no flags at all. layout_symtab() sets SHF_ALLOC on them so that move_module()
will copy them, and it places them itself at the end of MOD_INIT_DATA.

Without the clearing, __layout_sections() on the second pass would pick the two
sections up again: SHF_ALLOC set, no SHF_WRITE, so they match its RODATA mask and get
some of MOD_RODATA reserved. Then layout_symtab() runs anyway, overwrites sh_entsize
and puts them into MOD_INIT_DATA, same as the first pass. The reserved MOD_RODATA is
never used by anything, so the module would just carry that dead space for no reason.
Hence the clearing.

Or am I missing any details?

>>
>>> +             goto retry;
>>> +     }
>>>
>>>       /* Module has been copied to its final place now: return it. */
>>>       mod = (void *)info->sechdrs[info->index.mod].sh_addr;
>>
>> I'm not sure this is the best approach. It's complex logic for what
>> appears to be an edge case related to a debugging facility. It will have
>> the usual problem of error paths not getting enough testing and breaking
>> subtly over time.
>>
>> An alternative could be to reset SHF_ALLOC on the codetag section to
>> remove it from further processing and have relocations that point to
>> this section resolve to something else. It seems that alloc_hooks_tag()
>> could tolerate this, since it only needs to reference the associated
>> alloc_tag when mem_alloc_profiling_enabled() is true and that gets
>> disabled by reserve_module_tags() on the overflow.
> 
> Hmm, yeah if we redirect the references into .codetag.alloc_tags, that
> would be much better.
> 
>>
>> It is also not an ideal approach, but I feel it could be less intrusive
>> to the module loader. I can put together a prototype if needed.
> 
> If your approach does not cause module loading to fail when we disable
> profiling, then that sounds like a good idea. If it's not too much
> trouble, could you please send an RFC?
> 
>>
>> --
>> Thanks,
>> Petr