Re: [RFC PATCH 2/3] module: move codetag section placement decision to layout_sections()

Hi Suren

Thanks for your review.

On 2026/8/15 14:21, Suren Baghdasaryan wrote:
> On Thu, Aug 13, 2026 at 2:34 AM Hao Ge <[email protected]> wrote:
>> codetag_needs_module_section() is called twice per codetag section, once
>> in layout_sections() and once in move_module(), and both depend on
>> mem_profiling_support, which changes without a lock. If profiling is
>> disabled between the two calls, layout excludes the section (offset 0)
>> while move copies it as normal memory to offset 0:
>>
>>   CPU0 (insmod A)                    CPU1 (insmod B)
>>   ----------------                   ----------------
>>   layout_sections()
>>     needs_section_mem() == true
>>     sh_entsize: type, offset = 0
>>                                      reserve_module_tags() overflows
>>                                        shutdown_mem_profiling()
>>                                        mem_profiling_support = false
>>   move_module()
>>     needs_section_mem() == false
>>     offset = sh_entsize & MASK = 0
>>     memcpy(mod->mem[type].base + 0, ...)
>>        -> overwrites the first section there
>>
>> Record the decision in layout_sections() in sh_entsize using a
>> MOD_MEM_CODETAG type, and have move_module() use that instead of asking
>> again.
>>
>> reserve_module_tags() returns -EAGAIN if profiling was disabled after
>> layout, so the loader retries and places the section as normal memory.
>>
>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag compression")
>> Signed-off-by: Hao Ge <[email protected]>
>> ---
>>  include/linux/module.h | 11 +++++++++++
>>  kernel/module/main.c   | 17 ++++++-----------
>>  mm/alloc_tag.c         |  8 ++++++++
>>  3 files changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/linux/module.h b/include/linux/module.h
>> index 7566815fabbe..a02016528e1d 100644
>> --- a/include/linux/module.h
>> +++ b/include/linux/module.h
>> @@ -328,6 +328,17 @@ enum mod_mem_type {
>>         MOD_INVALID = -1,
>>  };
>>
>> +/*
>> + * If CONFIG_CODE_TAGGING is on, modules get a .codetag section.
>> + * codetag_needs_module_section() says where it goes: the usual
>> + * mod->mem[], or off to the codetag region.
>> + *
>> + * Mark the codetag-region ones with MOD_MEM_NUM_TYPES.
>> + * It's just past the real types, so it doesn't index into mod->mem[]
>> + * and for_each_mod_mem_type() skips it.
>> + */
>> +#define MOD_MEM_CODETAG        MOD_MEM_NUM_TYPES
> Ok, it feels a bit hacky but it's probably the simplest way to mark
> codetag regions.
> 

Yeah, it's not the prettiest — that's part of why I sent this series
as an RFC. Actually, I was wondering whether we could turn this into a
generic marker which means the section does not live in mod->mem[] but
in a region managed by its owning subsystem, codetag being the first
user. Of course, we still need to consult the module subsystem experts
about this.

>> +
>>  #define mod_mem_type_is_init(type)     \
>>         ((type) == MOD_INIT_TEXT ||     \
>>          (type) == MOD_INIT_DATA ||     \
>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>> index ed26f167be84..2337bf604f58 100644
>> --- a/kernel/module/main.c
>> +++ b/kernel/module/main.c
>> @@ -1728,11 +1728,8 @@ static void __layout_sections(struct module *mod, struct load_info *info, bool i
>>                          * preallocated contiguous memory.
>>                          */
>>                         if (codetag_needs_module_section(mod, sname, s->sh_size)) {
>> -                               /*
>> -                                * s->sh_entsize won't be used but populate the
>> -                                * type field to avoid confusion.
>> -                                */
>> -                               s->sh_entsize = ((unsigned long)(type) & SH_ENTSIZE_TYPE_MASK)
>> +                               s->sh_entsize = ((unsigned long)MOD_MEM_CODETAG
>> +                                                & SH_ENTSIZE_TYPE_MASK)
>>                                                 << SH_ENTSIZE_TYPE_SHIFT;
>>                                 continue;
>>                         }
>> @@ -2815,11 +2812,10 @@ static int move_module(struct module *mod, struct load_info *info)
>>                         continue;
>>
>>                 sname = info->secstrings + shdr->sh_name;
>> -               /*
>> -                * Load codetag sections separately as they might still be used
>> -                * after module unload.
>> -                */
>> -               if (codetag_needs_module_section(mod, sname, shdr->sh_size)) {
>> +
>> +               enum mod_mem_type type = shdr->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT;
>> +
>> +               if (type == MOD_MEM_CODETAG) {
>>                         dest = codetag_alloc_module_section(mod, sname, shdr->sh_size,
>>                                         arch_mod_section_prepend(mod, i), shdr->sh_addralign);
>>                         if (WARN_ON(!dest)) {
>> @@ -2832,7 +2828,6 @@ static int move_module(struct module *mod, struct load_info *info)
>>                         }
>>                         codetag_section_found = true;
>>                 } else {
>> -                       enum mod_mem_type type = shdr->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT;
>>                         unsigned long offset = shdr->sh_entsize & SH_ENTSIZE_OFFSET_MASK;
>>
>>                         dest = mod->mem[type].base + offset;
>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
>> index 461fa87fbb0b..7481180dadd2 100644
>> --- a/mm/alloc_tag.c
>> +++ b/mm/alloc_tag.c
>> @@ -893,6 +893,14 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>>         if (size < sizeof(struct alloc_tag))
>>                 return ERR_PTR(-EINVAL);
>>
>> +       /*
>> +        * Profiling may have been disabled by a concurrent module load.
>> +        * Return -EAGAIN so the loader retries with profiling off, laying
>> +        * the section out as ordinary module memory.
>> +        */
>> +       if (!mem_profiling_support)
>> +               return ERR_PTR(-EAGAIN);
> I think this requires your patch [1] from another patchset to work
> correctly, correct? If so, I would suggest sending this patch as part
> of that patchset since there is a dependency.
> 

Right, this hunk needs the retry from [1] to work. I'll drop this
patch from this series and send it as part of that patchset instead.

I'll wait until we sort out the plan for [1] with Petr before pushing this.
Feedback from the module‑subsystem experts on this patch is most welcome.

> [1] https://lore.kernel.org/all/[email protected]/
>
> 
>> +
>>         /*
>>          * align is always power of 2, so we can use IS_ALIGNED and ALIGN.
>>          * align 0 or 1 means no alignment, to simplify set to 1.
>> --
>> 2.25.1