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

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

New issues:
- [High] Unconditional allocation of per-CPU counters for modules causes a memory leak when memory profiling is disabled and the module load is retried.

Pre-existing issues:
- [Critical] Concurrent module loading can cause an out-of-bounds memory write and module corruption because the global profiling flag is toggled without cross-phase synchronization.
- [High] A race condition on the global module tags size variable can lead to unmapped virtual memory access and a kernel panic.
- [High] If page population fails, a dangling module pointer is left in the maple tree, leading to a Use-After-Free.
--

--- Patch [2]: [PATCH v5 2/2] alloc_tag: fix undetected compressed tag overflow when profiling is disabled ---
commit ffb2b208763f32cdfb3d6cb740d4d4efa5c99ae0
Author: Hao Ge <[email protected]>

alloc_tag: fix undetected compressed tag overflow when profiling is disabled

This patch modifies module layout and allocation to retry loading without
memory profiling if the compressed tag limit is exceeded. It removes the
profiling enabled guard when checking for tag overflow and handles the
fallback by resetting section flags and repeating the layout process.

> diff --git a/kernel/module/main.c b/kernel/module/main.c
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c

[ ... ]

> @@ -2966,18 +2967,38 @@ static struct module *layout_and_allocate(struct load_info *info, int flags)
>  	 */
>  	module_mark_ro_after_init(info->hdr, info->sechdrs, info->secstrings);
>  
> +	/*
> +	 * Save the sizes reserved by module_frob_arch_sections() so they can
> +	 * be restored if we retry below.
> +	 */
> +	for_each_mod_mem_type(type)
> +		frob_size[type] = info->mod->mem[type].size;
> +
>  	/*
>  	 * Determine total sizes, and put offsets in sh_entsize.  For now
>  	 * 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: Critical]
This isn't a bug introduced by this patch, but can concurrent module loading
cause memory corruption if profiling is disabled mid-flight?

In move_module(), if one thread disables profiling globally via
shutdown_mem_profiling() while another thread is halfway through module
loading (between layout calculation and memory movement), the second thread's
section offset queries could evaluate to false.

This would cause the module loader to use an un-incremented 0 offset and
incorrectly write the codetag section into the base memory, overwriting the
start of the module's core memory block.

> -	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 = frob_size[type];
> +		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]
Does this retry mechanism cause a memory leak for per-CPU counters?

When the module load is retried via this new loop, the codetag section is
treated as standard memory and bypasses maple tree registration. However, the
subsequent codetag module initialization unconditionally allocates per-CPU
memory for these tags regardless of the profiling state.

During module unload in release_module_tags(), the cleanup path exits early
because the module cannot be found in the maple tree, permanently leaking
the per-CPU memory on every unload.

> +	}
>  
>  	/* 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
> --- a/mm/alloc_tag.c
> +++ b/mm/alloc_tag.c

[ ... ]

> @@ -950,10 +950,12 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>  		int grow_res;
>  
>  		module_tags.size = offset + size;

[Severity: High]
This is a pre-existing issue, but does the update to module_tags.size race
with concurrent module loads?

The high-water mark for the module tags size is updated here without a lock
after the maple tree lock is released. If two concurrent module loads race on
this update, a higher offset might be overwritten by a smaller one.

Subsequently, vm_module_tags_populate() relies on this corrupted size and
would fail to map the physical pages for the higher virtual addresses, leading
to a page fault when the module accesses its allocated tags.

> -		if (mem_alloc_profiling_enabled() && !tags_addressable()) {
> +		if (!tags_addressable()) {
>  			shutdown_mem_profiling(true);
> -			pr_warn("With module %s there are too many tags to fit in %d page flag bits. Memory allocation profiling is disabled!\n",
> -				mod->name, NR_UNUSED_PAGEFLAG_BITS);
> +			pr_warn_once("With module %s there are too many tags to fit in %d page flag bits. Memory allocation profiling is disabled!\n",
> +				     mod->name, NR_UNUSED_PAGEFLAG_BITS);
> +			release_module_tags(mod, false);
> +			return ERR_PTR(-EAGAIN);
>  		}
>  
>  		grow_res = vm_module_tags_populate();

[Severity: High]
This is a pre-existing issue, but could this lead to a dangling pointer in
the maple tree if page population fails?

If vm_module_tags_populate() fails and returns an error, the allocation
routine returns an error here but doesn't remove the newly inserted module
pointer from the maple tree.

Since this error bypasses the standard codetag cleanup routines in the module
loader (where codetag_section_found might remain false in move_module()),
the maple tree retains a pointer to the aborted module. Once the module
structure is freed, future maple tree iterations could dereference this freed
pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2