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

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

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?


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/
>