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

[SeongJae Park <sj@kernel.org>]

On Thu, 14 May 2026 02:18:36 +0000 sashiko-bot@kernel.org wrote:

> 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.

TL; DR: Sashiko found upstream commit fed48693bdfe ("mm/damon/reclaim: use
parameter context correctly") is required for 6.12.y regardless of this patch,
but not yet picked by stable kernel maintainers.  Stable kernel maintainers,
could you please add the patch to 6.12.y?

Other Sashiko-found issues are no issue in my opinion.

Please read below if you are interested in more details.

> --
> 
> 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?

The bug is introduced by commit 11ddcfc257a3 ("mm/damon/reclaim: use
damon_commit_ctx()"), which was merged into the mainline by 6.11.  So the
broken commit is in 6.12.y.

The fix was made by commit fed48693bdfe ("mm/damon/reclaim: use parameter
context correctly") which was merged into the mainline by 6.17.  And the fix
commit has correct Fixes: tag, but somehow didn't picked by stable kernel
maintainers.

Stable kernel maintainers, could you please backport the fix commit to 6.12.y?
Let me know if any different action from my side is needed.

> 
> >  	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?

I informed this and where the dependent patches can be found, on the commentary
area of the patch.

> 
> >  
> >  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?

DAMON_RECLAIM is a static module.  Userspace cannot write sysfs files before
the static module initializations are finished to my understanding.  So this
kind of race cannot happen.  Please let me know if I'm wrong.

> 
> [ ... ]
> 
> > +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?

I informed this and where the dependent patches can be found on the commentary
area of this patch.

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


Thanks,
SJ