Re: [RFC PATCH] mm/damon/stat: detect and use fresh enabled status

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [Medium] Writing 'N' to the `enabled` parameter after kdamond has stopped unexpectedly bypasses `damon_stat_stop()`, leaking `damon_stat_context`.
--

commit 1171ea4d01e310ec9987bf3cb2d19f8bf89b60cb
Author: SeongJae Park <sj@kernel.org>

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

This commit changes the enabled parameter's get callback to report the
actual running status of kdamond instead of the static enabled variable.
It introduces damon_stat_enabled() to dynamically check if the context is
running.

> 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 stops unexpectedly (for example, due to an allocation failure in
kdamond_fn), damon_is_running() and consequently damon_stat_enabled() will
return false.

If a user then explicitly disables the module by writing 'N' to the enabled
parameter, kstrtobool() will update the global enabled variable to false.

Could this cause the check above to evaluate to true (false == false) and
return 0 early?

>  
>  	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();
>  	damon_stat_stop();

If the early return is taken, damon_stat_stop() is bypassed. Does this leak
damon_stat_context since damon_destroy_ctx() won't be called?

>  	return 0;
>  }

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