Re: [PATCH v4 2/2] alloc_tag: fix undetected compressed tag overflow when profiling is disabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Skipping `tag->counters` allocation leads to invalid memory access (garbage reads or crashes) when reading tags.
- [High] The retry loop skips `module_frob_arch_sections()`, causing module memory corruption on architectures that allocate space inside it.

Pre-existing issues:
- [High] Concurrent module loading can lead to silent module memory corruption if allocation profiling is dynamically disabled.
--

--- Patch [2]: [PATCH v4 2/2] alloc_tag: fix undetected compressed tag overflow when profiling is disabled ---
commit 9e076bfe2df23289b04f8850ce26739d08894c81
Author: Hao Ge <hao.ge@linux.dev>

alloc_tag: fix undetected compressed tag overflow when profiling is disabled

This commit removes the mem_alloc_profiling_enabled() guard when checking for
tag overflows in reserve_module_tags(). If an overflow occurs, it shuts down
profiling and uses an -EAGAIN retry loop in layout_and_allocate() to load the
module without profiling. It also skips allocating per-cpu counters if profiling
is disabled.

> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 46dd8d25a6058..c32bb47a573a1 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
[ ... ]
> @@ -2971,13 +2971,26 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>  	 * this is done generically; there doesn't appear to be any
>  	 * special cases for the architectures.
>  	 */
> +retry:
>  	layout_sections(info->mod, info);
>  	layout_symtab(info->mod, info);
>  
>  	/* Allocate and move to the final place */
>  	err = move_module(info->mod, info);

[Severity: High]
This isn't a bug introduced by this patch, but could concurrent module loading
lead to silent memory corruption if allocation profiling is dynamically
disabled?

When two modules load concurrently, Module A might calculate section offsets
in layout_sections() while mem_profiling_support is true. Its .alloc_tags
section gets a 0 offset since codetag_needs_module_section() evaluates to
true.

Concurrently, Module B might trigger a tag overflow in reserve_module_tags(),
which calls shutdown_mem_profiling() and sets mem_profiling_support to false
locklessly.

When Module A reaches move_module(), codetag_needs_module_section() now
evaluates to false. Does this cause the fallback path to place the section
at offset 0 within the module's core memory block, overwriting the first
valid section?

> -	if (err)
> -		return ERR_PTR(err);
> +	if (err) {
> +		if (err != -EAGAIN)
> +			return ERR_PTR(err);
> +		/*
> +		 * -EAGAIN means profiling was disabled but the module
> +		 * can still load without it. Reset state and retry.
> +		 */
> +		rewrite_section_headers(info, flags);
> +		for_each_mod_mem_type(type)
> +			info->mod->mem[type].size = 0;
> +		info->sechdrs[info->index.sym].sh_flags &= ~(unsigned long)SHF_ALLOC;
> +		info->sechdrs[info->index.str].sh_flags &= ~(unsigned long)SHF_ALLOC;
> +		goto retry;

[Severity: High]
Since the retry loop zeroes mod->mem[type].size and jumps to the retry label,
does it skip module_frob_arch_sections() which was called earlier in
layout_and_allocate()?

Architectures like s390 use module_frob_arch_sections() to reserve space at
the beginning of MOD_TEXT by modifying mod->mem[MOD_TEXT].size and caching
offsets:

arch/s390/kernel/module.c:module_frob_arch_sections() {
    ...
    mod_mem = &me->mem[MOD_TEXT];
    mod_mem->size = ALIGN(mod_mem->size, 4);
    me->arch.got_offset = mod_mem->size;
    mod_mem->size += me->arch.got_size;
    ...
}

Will skipping this initialization on retry cause the architecture loader to
place normal ELF sections at offset 0, and then overwrite the executable
.text section later during relocation using the cached offsets?

> +	}
>  
>  	/* Module has been copied to its final place now: return it. */
>  	mod = (void *)info->sechdrs[info->index.mod].sh_addr;
[ ... ]
> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> index 96a9196635fea..3c81ec7988860 100644
> --- a/mm/alloc_tag.c
> +++ b/mm/alloc_tag.c
[ ... ]
> @@ -633,6 +635,9 @@ static int load_module(struct module *mod, struct codetag *start, struct codetag
>  	struct alloc_tag *stop_tag;
>  	struct alloc_tag *tag;
>  
> +	if (!mem_profiling_support)
> +		return 0;

[Severity: High]
If we return 0 here, does it skip allocating tag->counters while still
allowing the module to load with its .alloc_tags section processed as
regular data?

Later, when show_mem() is triggered or /proc/allocinfo is read,
alloc_tag_top_users() unconditionally iterates over the global tags:

mm/alloc_tag.c:alloc_tag_top_users() {
    ...
    while ((ct = codetag_next_ct(&iter))) {
        struct alloc_tag_counters counter = alloc_tag_read(ct_to_alloc_tag(ct));
    ...
}

Will calling alloc_tag_read() dereference the NULL counter?

include/linux/alloc_tag.h:alloc_tag_read() {
    ...
    counter = per_cpu_ptr(tag->counters, cpu);
    v.bytes += counter->bytes;
    ...
}

Because per_cpu_ptr doesn't check for NULL, won't this add the per-cpu
offset to 0 and attempt to read unmapped memory or garbage data, causing a
kernel crash?

> +
>  	/* percpu counters for core allocations are already statically allocated */
>  	if (!mod)
>  		return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810093955.153015-1-hao.ge@linux.dev?part=2