Re: [RFC PATCH 3/3] alloc_tag: remove /proc/allocinfo outside of mod_lock

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

Pre-existing issues:
- [High] TOCTOU race in sysctl handler allows memory profiling to remain active without support structures.
--

--- Patch [3]: [RFC PATCH 3/3] alloc_tag: remove /proc/allocinfo outside of mod_lock ---
commit 8d4c5cdeb441b4a47f3511f205a68485820706b0
Author: Hao Ge <[email protected]>

alloc_tag: remove /proc/allocinfo outside of mod_lock

shutdown_mem_profiling() calls remove_proc_entry() from
reserve_module_tags(), which runs under mod_lock held for write.
remove_proc_entry() waits for readers, and a reader takes mod_lock for
read in allocinfo_start(). Move remove_proc_entry() to a workqueue.

> diff --git a/mm/alloc_tag.c b/mm/alloc_tag.c
> index 26c6ebc96a6ae..65d0ae9af94b2 100644
> --- a/mm/alloc_tag.c
> +++ b/mm/alloc_tag.c

[ ... ]

> @@ -600,7 +607,7 @@ static void shutdown_mem_profiling(bool remove_file)
>  		return;
>  
>  	if (remove_file)
> -		remove_proc_entry(ALLOCINFO_FILE_NAME, NULL);
> +		schedule_work(&remove_allocinfo_work);
>  	mem_profiling_support = false;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does shutdown_mem_profiling()
have a TOCTOU race with the sysctl handler proc_mem_profiling_handler()?

If a concurrent module load fails setup and calls shutdown_mem_profiling(),
it disables the static key and sets mem_profiling_support = false.

Concurrently, could a sysctl write to /proc/sys/vm/mem_profiling evaluate
mem_profiling_support before it is set to false:

mm/alloc_tag.c:proc_mem_profiling_handler() {
    ...
        if (!mem_profiling_support)
            return -EINVAL;
    }

    return proc_do_static_key(table, write, buffer, lenp, ppos);
}

and then execute proc_do_static_key() after shutdown_mem_profiling() has
finished, errantly re-enabling the static key?

If the static key is enabled but mem_profiling_support is false, any
subsequent module load will skip allocating per-cpu tag counters. When
the new module executes a memory allocation, would alloc_tag_ref_set()
unconditionally dereference a NULL tag->counters?

include/linux/alloc_tag.h:alloc_tag_ref_set() {
    ...
    this_cpu_inc(tag->counters->calls);
    ...
}

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