Re: [PATCH 6.12.y] mm/damon/reclaim: detect and use fresh enabled and kdamond_pid values

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] A race condition during module initialization allows userspace to trigger a use-after-free kernel panic via the `enabled` sysfs parameter.
- [High] `damon_reclaim_apply_parameters()` incorrectly applies schemes and attributes to the global `ctx` instead of `param_ctx`, causing them to be destroyed by `damon_commit_ctx()`.
- [Low] The patch introduces calls to undeclared functions `damon_is_running()` and `damon_kdamond_pid()`, causing a build failure.
--

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

mm/damon/reclaim: detect and use fresh enabled and kdamond_pid values

This commit updates DAMON_RECLAIM to dynamically detect and use the fresh
running status and process id for the enabled and kdamond_pid parameters.
It avoids caching the status, which can become stale when the thread stops
unexpectedly.

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

> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> index 65842e6854fd1..9df096218beb7 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c
> @@ -247,60 +238,93 @@ static int damon_reclaim_turn(bool on)
>  {
>  	int err;
>  
> -	if (!on) {
> -		err = damon_stop(&ctx, 1);
> -		if (!err)
> -			kdamond_pid = -1;
> -		return err;
> -	}
> +	if (!on)
> +		return damon_stop(&ctx, 1);
>  
>  	err = damon_reclaim_apply_parameters();

Does damon_reclaim_apply_parameters() correctly configure the scheme?

Looking at its implementation in mm/damon/reclaim.c:

    err = damon_modules_new_paddr_ctx_target(&param_ctx, &param_target);
    ...
    err = damon_set_attrs(ctx, &damon_reclaim_mon_attrs);
    ...
    scheme = damon_reclaim_new_scheme();
    ...
    damon_set_schemes(ctx, &scheme, 1);
    ...
    err = damon_commit_ctx(ctx, param_ctx);

It seems damon_set_attrs() and damon_set_schemes() are called on the global
ctx rather than param_ctx.

Since param_ctx has zero schemes when damon_commit_ctx(ctx, param_ctx) is
called, will this aggressively destroy the schemes that were just added to
ctx, leaving DAMON_RECLAIM non-functional?

>  	if (err)
>  		return err;
>  
> -	err = damon_start(&ctx, 1, true);
> -	if (err)
> -		return err;
> -	kdamond_pid = ctx->kdamond->pid;
> -	return 0;
> +	return damon_start(&ctx, 1, true);
> +}
> +
> +static bool damon_reclaim_enabled(void)
> +{
> +	if (!ctx)
> +		return false;
> +	return damon_is_running(ctx);
> +}

Does this code compile?

It appears that damon_is_running() was introduced in a newer kernel version.
Because this is a cherry-pick, are we missing a prerequisite refactoring
patch that provides this function?

>  
>  static int damon_reclaim_enabled_store(const char *val,
>  		const struct kernel_param *kp)
>  {
> -	bool is_enabled = enabled;
> -	bool enable;
>  	int err;
>  
> -	err = kstrtobool(val, &enable);
> +	err = kstrtobool(val, &enabled);
>  	if (err)
>  		return err;
>  
> -	if (is_enabled == enable)
> +	if (damon_reclaim_enabled() == enabled)
>  		return 0;
>  
>  	/* Called before init function.  The function will handle this. */
>  	if (!ctx)
> -		goto set_param_out;
> +		return 0;
>  
> -	err = damon_reclaim_turn(enable);
> -	if (err)
> -		return err;
> +	return damon_reclaim_turn(enabled);
> +}

Is there a race condition between module initialization and this sysfs
parameter store?

The sysfs file for enabled is exposed to userspace by load_module() before
damon_reclaim_init() completes. damon_reclaim_init() allocates ctx early.

If userspace writes to enabled after ctx is allocated but before
initialization finishes, this function will start the kdamond thread.

When damon_reclaim_init() resumes, it will see enabled is true and attempt to
start the thread again, resulting in -EBUSY.

If initialization fails and the module memory is freed, could the
already-running kdamond thread execute unmapped memory and cause a
use-after-free regression?

[ ... ]

> +static int damon_reclaim_kdamond_pid_load(char *buffer,
> +		const struct kernel_param *kp)
> +{
> +	int kdamond_pid = -1;
> +
> +	if (ctx) {
> +		kdamond_pid = damon_kdamond_pid(ctx);
> +		if (kdamond_pid < 0)
> +			kdamond_pid = -1;
> +	}
> +	return sprintf(buffer, "%d\n", kdamond_pid);
> +}

Likewise, is damon_kdamond_pid() available in this kernel tree, or will this
result in an implicit function declaration error?

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