Re: [RFC PATCH 1/3] alloc_tag: skip percpu counter allocation when profiling is disabled

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

Pre-existing issues:
- [High] Missing error code assignment when codetag_load_module() fails in load_module().
- [High] Use-After-Free and memory leak due to missing codetag_unload_module() on the module load error path.
--

--- Patch [1]: [RFC PATCH 1/3] alloc_tag: skip percpu counter allocation when profiling is disabled ---
commit 355b88475bea314f9aab64a9aa73da39a0d2bcf0
Author: Hao Ge <[email protected]>

alloc_tag: skip percpu counter allocation when profiling is disabled

After shutdown_mem_profiling() clears mem_profiling_support,
needs_section_mem() returns false, so later modules have their codetag
section placed as regular data and never enter the alloc_tag maple tree.
This patch returns CODETAG_MODULE_EXCLUDED when profiling is off to skip
percpu counter allocation.

> diff --git a/lib/codetag.c b/lib/codetag.c
> index a9cda4c962a30..8506ecab9ea70 100644
> --- a/lib/codetag.c
> +++ b/lib/codetag.c
[ ... ]
> @@ -238,9 +238,10 @@ static int codetag_module_init(struct codetag_type *cttype, struct module *mod)
>  	}
>  	up_write(&cttype->mod_lock);
>  
> -	if (err < 0) {
> +	if (err) {
> +		/* Error or excluded: cmod is dropped, free it. */
>  		kfree(cmod);
> -		return err;
> +		return err < 0 ? err : 0;
>  	}
>  
>  	return 0;

[Severity: High]
This is a pre-existing issue, but if codetag_module_init() fails midway through
the codetag types, does it leave the module registered in the previously
initialized types?

lib/codetag.c:codetag_load_module() {
...
	list_for_each_entry(cttype, &codetag_types, link) {
		ret = codetag_module_init(cttype, mod);
		if (ret)
			break;
	}
...
}

If the loop breaks on an error, the successfully initialized types are not
unwound. When load_module() in kernel/module/main.c handles this error, it
jumps to sysfs_cleanup and eventually frees the module, but misses a call to
codetag_unload_module(mod). 

Could this lead to a use-after-free or a memory leak when traversing the IDR
later, since the freed module is still registered?


[Severity: High]
This is another pre-existing issue. Is the error code lost in load_module()
when codetag_load_module() fails?

kernel/module/main.c:load_module() {
...
	if (codetag_load_module(mod))
		goto sysfs_cleanup;
...
}

Since err is not assigned before jumping to sysfs_cleanup, would the syscall
incorrectly return 0 (success) even though the module failed to load and was
destroyed on the error path?

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