Re: [RFC PATCH v2.1 3/3] mm/damon/stat: detect and use fresh enabled value

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] Bypassing damon_stat_stop() and overwriting damon_stat_context causes a memory leak when explicitly disabling or restarting a kdamond thread that stopped internally.
--

commit b9703c85b8e9f48ea8dc673102aa2ecc0e1ac448
Author: SeongJae Park <sj@kernel.org>
mm/damon/stat: detect and use fresh enabled value

This commit dynamically detects the status of kdamond instead of relying on
the saved parameter value to avoid showing stale data when the daemon stops
internally.

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;

Does this early return cause a memory leak if kdamond stops internally?

If kdamond stops internally, damon_is_running() will return false, causing
damon_stat_enabled() to also return false. However, damon_stat_context
remains allocated.

If a user subsequently writes 'N' to the enabled parameter to clean up,
damon_stat_enabled() == enabled evaluates to false == false. This triggers
the early return here, bypassing damon_stat_stop() which would have
called damon_destroy_ctx(damon_stat_context) to free the memory.

Similarly, if a user writes 'Y' to restart the module, damon_stat_start()
will be called. Looking at damon_stat_start(), it allocates a new context
by calling damon_stat_build_ctx() and overwrites the damon_stat_context
pointer without freeing the previous one.

Can this leak the context in both scenarios?

>  
>  	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();
>  	return 0;
>  }

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