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

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

Hi Petr

On 2026/9/4 23:25, Petr Pavlu wrote:
> 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.
>

Yes, I used mod_percpu() to stay consistent with /proc/kallsyms.

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

OK. I spent the morning looking for relevant specs, but found none.
Nor is this documented in Documentation/ABI/testing/sysfs-module.

https://elixir.bootlin.com/linux/v7.2.2/source/Documentation/ABI/testing/sysfs-module
https://elixir.bootlin.com/linux/v7.2.2/source/Documentation/ABI/stable/sysfs-module

But I've noticed similar usage in __is_module_percpu_address,
https://elixir.bootlin.com/linux/v7.2.2/source/kernel/module/main.c#L516

So I will first add documentation for this in Documentation/ABI/testing/sysfs-module.

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

Agree.

Thanks for your help.

Best Regards
Hao