Re: [PATCH v9 3/4] module: introduce SH_ENTSIZE_STANDALONE for separately allocated sections

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

On 9/9/26 20:47, Hao Ge wrote:
> Hi Petr
>
>
> On 9/9/26 19:30, Petr Pavlu wrote:
>> On 9/8/26 11:24 AM, Hao Ge wrote:
>>> SHF_ALLOC means, per the ELF spec, that a section occupies memory
>>> during process execution. Some module sections occupy memory
>>> outside the regular module layout, for example the percpu section
>>> with its per-CPU allocations. The loader currently excludes such
>>> a section from the layout by clearing its SHF_ALLOC, which
>>> overloads the flag with a loader-internal meaning.
>>> apply_relocations() needs a special case for the section, and
>>> find_sec(".data..percpu") returns different results before and
>>> after layout_and_allocate().
>>>
>>> Introduce SH_ENTSIZE_STANDALONE to mark sections with a separate
>>> allocation. The percpu section is its first user. layout_sections()
>>> and move_module() skip marked sections, and apply_relocations() goes
>>> back to testing only SHF_ALLOC. Based on a patch by Petr Pavlu [1].
>>>
>>> .data..percpu keeps SHF_ALLOC, so it would now show up under
>>> /sys/module/*/sections/. The section has one instance per CPU and no
>>> single address to report, and the entry never existed before, so
>>> skip it in add_sect_attrs(). add_notes_attrs() indexes its attrs[]
>>> array and skips it too. No functional change otherwise.
>>>
>>> Fixes: 4835f747d3ed ("alloc_tag: support for page allocation tag 
>>> compression")
>>> Reported-by: Sashiko <sashiko-bot@kernel.org>
>>> Link: 
>>> https://lore.kernel.org/all/499bb60c-c6e3-43a3-bd92-95a0567ece5e@suse.com/ 
>>> [1]
>>> Suggested-by: Petr Pavlu <petr.pavlu@suse.com>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Hao Ge <hao.ge@linux.dev>
>>> ---
>>> [...]
>>> diff --git a/kernel/module/kallsyms.c b/kernel/module/kallsyms.c
>>> index 0fc11e45df9b..49190deae61e 100644
>>> --- a/kernel/module/kallsyms.c
>>> +++ b/kernel/module/kallsyms.c
>>> @@ -76,7 +76,7 @@ static char elf_type(const Elf_Sym *sym, const 
>>> struct load_info *info)
>>>   }
>>>     static bool is_core_symbol(const Elf_Sym *src, const Elf_Shdr 
>>> *sechdrs,
>>> -               unsigned int shnum, unsigned int pcpundx)
>>> +               unsigned int shnum)
>>>   {
>>>       const Elf_Shdr *sec;
>>>       enum mod_mem_type type;
>>> @@ -86,11 +86,6 @@ static bool is_core_symbol(const Elf_Sym *src, 
>>> const Elf_Shdr *sechdrs,
>>>           !src->st_name)
>>>           return false;
>>>   -#ifdef CONFIG_KALLSYMS_ALL
>>> -    if (src->st_shndx == pcpundx)
>>> -        return true;
>>> -#endif
>>> -
>>>       sec = sechdrs + src->st_shndx;
>>>       type = sec->sh_entsize >> SH_ENTSIZE_TYPE_SHIFT;
>>>       if (!(sec->sh_flags & SHF_ALLOC)
>>> @@ -131,8 +126,7 @@ void layout_symtab(struct module *mod, struct 
>>> load_info *info)
>>>       /* Compute total space required for the core symbols' strtab. */
>>>       for (ndst = i = 0; i < nsrc; i++) {
>>>           if (i == 0 || is_livepatch_module(mod) ||
>>> -            is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
>>> -                   info->index.pcpu)) {
>>> +            is_core_symbol(src + i, info->sechdrs, 
>>> info->hdr->e_shnum)) {
>>>               strtab_size += strlen(&info->strtab[src[i].st_name]) + 1;
>>>               ndst++;
>>>           }
>>> @@ -199,8 +193,7 @@ void add_kallsyms(struct module *mod, const 
>>> struct load_info *info)
>>>       for (ndst = i = 0; i < kallsyms->num_symtab; i++) {
>>>           kallsyms->typetab[i] = elf_type(src + i, info);
>>>           if (i == 0 || is_livepatch_module(mod) ||
>>> -            is_core_symbol(src + i, info->sechdrs, info->hdr->e_shnum,
>>> -                   info->index.pcpu)) {
>>> +            is_core_symbol(src + i, info->sechdrs, 
>>> info->hdr->e_shnum)) {
>>>               ssize_t ret;
>>>                 mod->core_kallsyms.typetab[ndst] =
>> FTR These changes in kernel/module/kallsyms.c have a conflict with the
>> series "Ignore local labels and mapping symbols during module load" [1],
>> which is currently queued on modules-next, but it should be
>> straightforward to resolve.
>>
>>> diff --git a/kernel/module/sysfs.c b/kernel/module/sysfs.c
>>> index 01c65d608873..f64170344e69 100644
>>> --- a/kernel/module/sysfs.c
>>> +++ b/kernel/module/sysfs.c
>>> @@ -62,6 +62,15 @@ static void free_sect_attrs(struct 
>>> module_sect_attrs *sect_attrs)
>>>       kfree(sect_attrs);
>>>   }
>>>   +/*
>>> + * .data..percpu has a separate allocation per CPU and no single
>>> + * address to report.
>>> + */
>>> +static bool sect_visible(const struct load_info *info, unsigned int i)
>>> +{
>>> +    return !sect_empty(&info->sechdrs[i]) && i != info->index.pcpu;
>>> +}
>>> +
>>>   static int add_sect_attrs(struct module *mod, const struct 
>>> load_info *info)
>>>   {
>>>       struct module_sect_attrs *sect_attrs;
>>> @@ -72,7 +81,7 @@ static int add_sect_attrs(struct module *mod, 
>>> const struct load_info *info)
>>>         /* Count loaded sections and allocate structures */
>>>       for (i = 0; i < info->hdr->e_shnum; i++)
>>> -        if (!sect_empty(&info->sechdrs[i]))
>>> +        if (sect_visible(info, i))
>>>               nloaded++;
>>>       sect_attrs = kzalloc_flex(*sect_attrs, attrs, nloaded);
>>>       if (!sect_attrs)
>>> @@ -92,7 +101,7 @@ static int add_sect_attrs(struct module *mod, 
>>> const struct load_info *info)
>>>       for (i = 0; i < info->hdr->e_shnum; i++) {
>>>           Elf_Shdr *sec = &info->sechdrs[i];
>>>   -        if (sect_empty(sec))
>>> +        if (!sect_visible(info, i))
>>>               continue;
>>>           sysfs_bin_attr_init(sattr);
>>>           sattr->attr.name =
>>> @@ -181,7 +190,7 @@ static int add_notes_attrs(struct module *mod, 
>>> const struct load_info *info)
>>>         nattr = &notes_attrs->attrs[0];
>>>       for (loaded = i = 0; i < info->hdr->e_shnum; ++i) {
>>> -        if (sect_empty(&info->sechdrs[i]))
>>> +        if (!sect_visible(info, i))
>>>               continue;
>>>           if (info->sechdrs[i].sh_type == SHT_NOTE) {
>>>               sysfs_bin_attr_init(nattr);
>> add_notes_attrs() has two sect_empty() calls. Both should be changed to
>> sect_visible().
>
>
> I kept this part unmodified to preserve the loop's original intent.
>
> This loop counts SHT_NOTE sections.
>
> SHT_NOTE refers to ELF note sections, which hold non-executable
>
> metadata such as build ID and ABI info.
>
> I wonder if we could keep the current implementation.
>
> As noted in the comment above, the top part counts SHT_NOTE sections
>
> and allocates structures, while the lower logic handles control of 
> node attributes.
>
>
> WDYT?
>
>

Sorry, I've reconsidered this. I think changing it to sect_visible would 
be better.

sect_visible stands for the count of externally visible note attributes, 
so the code

above and below can align with each other.


Sorry for the noise.


I'll rebase onto modules-next, apply this change and push a new revision.

I'll also add your Reviewed-by tag.


Thanks

Best Regards

Hao


> Thanks
>
> Best Regards
>
> Hao
>
>>
>> With this fixed, the patch looks ok to me. Feel free to add:
>>
>> Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
>>
>> [1] 
>> https://lore.kernel.org/linux-modules/20260820125007.22943-1-yangtiezhu@loongson.cn/
>>