On 9/9/26 3:08 PM, Hao Ge wrote:
> On 9/9/26 20:47, Hao Ge wrote:
>> 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 <[email protected]>
>>>> Link:
>>>> https://lore.kernel.org/all/[email protected]/
>>>> [1]
>>>> Suggested-by: Petr Pavlu <[email protected]>
>>>> Cc: [email protected]
>>>> Signed-off-by: Hao Ge <[email protected]>
>>>> ---
>>>> [...]
>>>> 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 = ¬es_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.
No worries.
One problem with continuing to use sect_empty() in the first loop is
that it could overallocate the number of required entries if a module
contains an SHT_NOTE section named .data..percpu. That shouldn't happen,
but we can trivially avoid it by using sect_visible() there as well.
Using the same condition in both loops also makes the code generally
simpler to understand.
--
Cheers,
Petr
