Re: [PATCH v3 3/3] mm/damon/stat: detect and use fresh enabled value

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] Passing `enabled=Y` during module load triggers a double-start, failing initialization and causing a Use-After-Free kernel panic.
- [High] Resource leak of `damon_stat_context` when explicitly disabling the module after an internal kdamond crash.
--

commit 16eae8a1bf380cdb61d4dd6b62187dd09f8ce859
Author: SeongJae Park <sj@kernel.org>

mm/damon/stat: detect and use fresh enabled value

This commit updates the DAMON_STAT module to dynamically detect the running
status of its kdamond thread rather than relying on a cached parameter
value.

Link: https://lore.kernel.org/20260416040602.88665-1-sj@kernel.org

> diff --git a/mm/damon/stat.c b/mm/damon/stat.c
> index 99ba346f9e325..3951b762cbddf 100644
> --- a/mm/damon/stat.c
> +++ b/mm/damon/stat.c
[ ... ]
> @@ -273,17 +276,23 @@ static void damon_stat_stop(void)
>  	damon_stat_context = NULL;
>  }
>  
> +static bool damon_stat_enabled(void)
> +{
> +	if (!damon_stat_context)
> +		return false;
> +	return damon_is_running(damon_stat_context);
> +}
> +
>  static int damon_stat_enabled_store(
>  		const char *val, const struct kernel_param *kp)
>  {
> -	bool is_enabled = enabled;
>  	int err;
>  
>  	err = kstrtobool(val, &enabled);
>  	if (err)
>  		return err;
>  
> -	if (is_enabled == enabled)
> +	if (damon_stat_enabled() == enabled)
>  		return 0;

If kdamond terminates asynchronously (for example, due to an allocation
failure), damon_stat_enabled() will return false.

If a user subsequently disables the module by writing 'N' to the enabled
parameter, this condition (false == false) evaluates to true and the
function returns 0.

Does this skip the call to damon_stat_stop() and leak the
damon_stat_context since damon_destroy_ctx() won't be called?

>  
>  	if (!damon_initialized())
> @@ -293,16 +302,17 @@ static int damon_stat_enabled_store(
>  		 */
>  		return 0;
>  
> -	if (enabled) {
> -		err = damon_stat_start();
> -		if (err)
> -			enabled = false;
> -		return err;
> -	}
> +	if (enabled)
> +		return damon_stat_start();

During module load via "insmod damon_stat.ko enabled=Y", parse_args() calls
this store function. Because damon_stat_enabled() dynamically returns false
at this point, the code proceeds to call damon_stat_start() to start the
kdamond thread.

Shortly after, the module initialization function damon_stat_init() runs,
sees enabled == true, and calls damon_stat_start() again. This second call
returns -EAGAIN because the context is already running.

The module initialization then fails, causing the kernel to unload the
module and free its memory.

Since the kdamond thread is still actively running in the background, can
this cause a Use-After-Free when the thread accesses the freed module
memory?

While this issue previously existed if CONFIG_DAMON_STAT_ENABLED_DEFAULT=n,
does this patch introduce the regression for the default configuration as
well?

>  	damon_stat_stop();
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260419161003.79176-1-sj@kernel.org?part=3