Re: [PATCH v8 4/4] alloc_tag: release the reservation when populate fails

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

On 2026/9/7 14:43, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> 
> New issues:
> - [Critical] The patch introduces a kernel crash by freeing the maple tree entry without rolling back module_tags.size on failure.
> 
> Pre-existing issues:
> - [High] vmap_pages_range() failure in vm_module_tags_populate() leaves physical pages mapped while freeing them, leading to Use-After-Free.
> - [High] AB-BA Deadlock between procfs read and module loading via shutdown_mem_profiling().
> - [Medium] Memory leak of percpu counters when module falls back to standard section allocation.
> --
> 
> commit 09d0b9820f6ab861e4c9680b44a685c8658fb571
> Author: Hao Ge <hao.ge@linux.dev>
> 
> alloc_tag: release the reservation when populate fails
> 
> vm_module_tags_populate() can fail after a reservation is stored in the
> maple tree. The error return previously leaked the entry because a failed
> load never unloads the module. This patch calls release_module_tags()
> on failure to free the reservation.
> 
>> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
>> index 112a014d4b890..c11813cd9e153 100644
>> --- a/mm/alloc_tag.c
>> +++ b/mm/alloc_tag.c
>> @@ -963,6 +963,7 @@ static void *reserve_module_tags(struct module *mod, unsigned long size,
>>  			shutdown_mem_profiling(true);
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but could calling
> shutdown_mem_profiling(true) here risk an AB-BA deadlock?
> 
> A read to /proc/allocinfo enters proc_reg_read_iter(), taking an 'in_use'
> reference on the proc entry, then calls allocinfo_start() which waits for
> down_read(&alloc_tag_cttype->mod_lock).
> 
> Concurrently, module loading calls codetag_alloc_module_section(), holding
> down_write(&alloc_tag_cttype->mod_lock).
> 
> If reserve_module_tags() encounters an error, it calls
> shutdown_mem_profiling(true), which invokes remove_proc_entry().
> 
> Since remove_proc_entry() blocks waiting for the 'in_use' reference count
> to drop to zero, and the procfs reader blocks on the mod_lock held by the
> writer, doesn't this create a system hang during a module load failure?
> 
>>  			pr_err("Failed to allocate memory for allocation tags in the module %s. Memory allocation profiling is disabled!\n",
>>  			       mod->name);
>> +			release_module_tags(mod, false);
> 
> [Severity: Critical]
> Will this new call to release_module_tags() cause a kernel crash on a
> subsequent module load?
> 
> In reserve_module_tags(), module_tags.size is optimistically updated
> (module_tags.size = offset + size;) before attempting to allocate physical
> memory via vm_module_tags_populate().
> 
> If population fails, this patch frees the maple tree entry, but
> module_tags.size is left artificially inflated.
> 
> A subsequent module load will search the maple tree and reuse this freed gap.
> Because module_tags.size is already inflated, the allocation check bypasses
> vm_module_tags_populate(), returning an unmapped virtual address. Can copying
> section data into this address then trigger a fatal page fault?
> 

I see.

No, not for a subsequent load. The failure path shuts profiling
down before returning:

grow_res = vm_module_tags_populate();
if (grow_res) {
	shutdown_mem_profiling(true); /* mem_profiling_support = false */
	...
	release_module_tags(mod, false);
	 return ERR_PTR(grow_res);
}

and any later load bails out in needs_section_mem() long before it
reaches reserve_module_tags():
	if (!mem_profiling_support)
		return false;          

so no new reservation is made and the unmapped address is never
handed out.

But this reminds me of another race condition:

CPU0: failing load                 	CPU1: concurrent load                                                                           
----------------------             	---------------------------
needs_section_mem(): true         	needs_section_mem(): true
reserve_module_tags(): reserve the gap,
	module_tags.size = end
	populate() fails, gap released
                                  	reserve_module_tags(): reuse the gap,
                                  	end <= size -> skip populate
                                  	memcpy() -> page fault

I have a fix for this, but I'd like to wait for Petr and Suren to review the
remaining parts before sending it out.

Thanks
Best Regards
Hao

> [Severity: Medium]
> This is a pre-existing issue, but might the use of release_module_tags()
> when falling back to a standard section allocation leak percpu counters?
> 
> If tags are not addressable, reserve_module_tags() calls
> release_module_tags(mod, false) to remove the module from the maple tree
> and returns -EAGAIN.
> 
> The module loader catches -EAGAIN and falls back to a normal section
> allocation. Later, load_module() successfully allocates percpu counters for
> the tags.
> 
> On unload, release_module_tags(mod, true) is called to free the counters.
> However, because the module is no longer in the maple tree, the search fails
> and exits early:
> 
> mm/alloc_tag.c:release_module_tags() {
>     ...
>     if (!val) /* module not found */
>         goto out;
>     ...
> }
> 
> Are the percpu counters allocated during load_module() permanently leaked
> in this scenario?
> 
>>  			return ERR_PTR(grow_res);
> 
> [Severity: High]
> This is a pre-existing issue, but is there a risk that the failure path of
> vm_module_tags_populate() leaves dangling PTEs causing a Use-After-Free?
> 
> In vm_module_tags_populate(), vmap_pages_range() maps allocated pages into
> the kernel virtual address space.
> 
> If it fails, it returns an error but may leave successfully mapped PTEs
> active in the page tables. The error path calls release_pages() to free the
> physical pages back to the allocator, but fails to call vunmap_range():
> 
> mm/alloc_tag.c:vm_module_tags_populate() {
>     ...
>     if (nr < more_pages || vmap_pages_range(...) < 0) {
>         release_pages(arg, nr);
>         return -ENOMEM;
>     }
>     ...
> }
> 
> Could this leave dangling PTEs with PAGE_KERNEL permissions pointing to
> freed pages, risking data corruption when the pages are reallocated?
> 
>>  		}
>>  	}
>