Re: [PATCH v7 2/4] module: introduce SH_ENTSIZE_STANDALONE for separately allocated sections

[Petr Pavlu <petr.pavlu@suse.com>]

On 9/3/26 8:00 AM, Hao Ge wrote:
> Hi Petr and Suren
> 
> On 2026/9/2 16:33, sashiko-bot@kernel.org wrote:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>> - [Medium] The `.data..percpu` section is incorrectly exported in sysfs with a dangling pointer to a freed temporary buffer.
>> --
>>
>> commit 5082dc654c1b8850af405eef473b249c0d936de9
>> Author: Hao Ge <hao.ge@linux.dev>
>>
>> module: introduce SH_ENTSIZE_STANDALONE for separately allocated sections
>>
>> This commit introduces a new marker, SH_ENTSIZE_STANDALONE, to indicate
>> sections with a separate allocation. Instead of clearing the SHF_ALLOC
>> flag, it utilizes sh_entsize so that sections with separate allocations
>> like the percpu section can be correctly skipped during layout.
>>
>> Link: https://lore.kernel.org/all/499bb60c-c6e3-43a3-bd92-95a0567ece5e@suse.com/
>>
>>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>>> index d0e1e0bd2ad06..b114b815187a9 100644
>>> --- a/kernel/module/main.c
>>> +++ b/kernel/module/main.c
>> [ ... ]
>>> @@ -2822,7 +2810,8 @@ static int move_module(struct module *mod, struct load_info *info)
>>>  		Elf_Shdr *shdr = &info->sechdrs[i];
>>>  		const char *sname;
>>>  
>>> -		if (!(shdr->sh_flags & SHF_ALLOC))
>>> +		if (!(shdr->sh_flags & SHF_ALLOC)
>>> +		    || shdr->sh_entsize == SH_ENTSIZE_STANDALONE)
>>>  			continue;
>>>  
>>>  		sname = info->secstrings + shdr->sh_name;
>> [ ... ]
>>> @@ -2967,8 +2957,13 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>>>  	if (err < 0)
>>>  		return ERR_PTR(err);
>>>  
>>> +	/* Repurpose sh_entsize to track where each section is allocated. */
>>> +	for (i = 0; i < info->hdr->e_shnum; i++)
>>> +		info->sechdrs[i].sh_entsize = ~0UL;
>>> +
>>>  	/* We will do a special allocation for per-cpu sections later. */
>>> -	info->sechdrs[info->index.pcpu].sh_flags &= ~(unsigned long)SHF_ALLOC;
>>> +	if (info->index.pcpu)
>>> +		info->sechdrs[info->index.pcpu].sh_entsize = SH_ENTSIZE_STANDALONE;
>>>  
>>>  	/*
>>>  	 * Mark relevant sections as SHF_RO_AFTER_INIT so layout_sections() can
>>
>> [Severity: Medium]
>> Because SHF_ALLOC is no longer cleared here, does this cause the
>> .data..percpu section to be incorrectly exported via sysfs?
>>
>> With SHF_ALLOC remaining intact, sect_empty() in kernel/module/sysfs.c now
>> returns false for the percpu section, causing it to be processed instead of
>> skipped.
>>
>> Additionally, because move_module() explicitly skips sections marked with
>> SH_ENTSIZE_STANDALONE, the sh_addr is never updated to the final allocated
>> memory.
>>
>> This means when add_sect_attrs() exports the section:
>>
>> kernel/module/sysfs.c:add_sect_attrs() {
>>     ...
>> 	if (sect_empty(sec))
>> 		continue;
>>     ...
>> 	sattr->private = (void *)sec->sh_addr;
>>     ...
>> }
>>
>> Will sattr->private end up storing an unmodified sh_addr pointing to the
>> freed temporary load_info buffer (info->hdr)?
>>
>> This appears to leak a stale vmalloc pointer address to userspace when
>> reading the sysfs file for the section.
>>
> Yes, Sashiko is correct. The fix is just one line after percpu_modcopy():
> 
> info->sechdrs[info->index.pcpu].sh_addr = (unsigned long)mod_percpu(mod); 

mod_percpu() (mod->percpu) isn't really the execution address of the
.data..percpu section. That section in the ELF file is only a template
to initialize the individual per-CPU variable areas.

I think the two options are:

1) Do not export the section under /sys/module/*/sections/.
2) Treat the boot CPU area as the canonical location of
   .data..percpu. In other words, do:
   info->sechdrs[info->index.pcpu].sh_addr = (unsigned long)per_cpu_ptr(mod->percpu, get_boot_cpu_id());

The /sys/module/*/sections/ interface is intended for debugging. From
that perspective, exposing the .data..percpu address for the boot CPU
may be useful and seems fairly reasonable to me. On the other hand, one
could argue that this can cause confusion, since a loaded module has
multiple .data..percpu instances.

I don't see a strong argument for either option, but I would lean toward
the second.

> But .data..percpu will show up in /sys/module/*/sections/, it was never
> exported before. Stable will get this too via backport, since later fixes
> depend on it.

I think exposing .data..percpu under /sys/module/*/sections/ is a minor
change and is fine for stable. As mentioned, /sys/module/*/sections/ is
a debugging interface. Additionally, most uses of per-CPU variables are
in the core kernel, with only a handful modules defining per-CPU data.
On my system, 30 modules out of 5383 contain a .data..percpu section.

-- 
Thanks,
Petr