Re: [PATCH 6.1.y 2/2] mm/damon/lru_sort: detect and use fresh enabled and kdamond_pid values

[SeongJae Park <sj@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] The patch completely fails to fix the restart issue, leaving the state machine deadlocked.
> - [High] Use-After-Free when reading sysfs parameters if module initialization fails.
> --
> 
> commit 975a161fef47e6bccc8ce6810404c2d8be4ff338
> Author: SeongJae Park <sj@kernel.org>
> Subject: mm/damon/lru_sort: detect and use fresh enabled and kdamond_pid values
> 
> This commit updates the DAMON_LRU_SORT module to dynamically detect and report
> the fresh status of enabled and kdamond_pid parameters. It attempts to address
> an issue where unintentional kdamond stoppages leave the system in a stale
> state, preventing users from restarting the daemon. By directly calling
> damon_is_running() and damon_kdamond_pid(), the system aims to provide
> accurate runtime information to sysfs readers.
> 
> Link: https://lore.kernel.org/20260419161003.79176-3-sj@kernel.org
> 
> > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > index 61311800abc98..70cdc7425cdd3 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c
> 
> [ ... ]
> 
> > @@ -285,6 +268,13 @@ static DECLARE_DELAYED_WORK(damon_lru_sort_timer, damon_lru_sort_timer_fn);
> >  
> >  static bool damon_lru_sort_initialized;
> >  
> > +static bool damon_lru_sort_enabled(void)
> > +{
> > +	if (!ctx)
> > +		return false;
> > +	return damon_is_running(ctx);
> > +}
> 
> Is it possible for this to cause a use-after-free if module initialization
> fails?
> 
> If damon_lru_sort_init() encounters an error (for example, damon_select_ops()
> fails),

So, damon_lru_sort_init() callse damon_modules_new_paddr_ctx_target(), which
calls damon_select_ops().  The damon_select_ops() cannot fail because
DAMON_LRU_SORT depends on DAMON_PADDR, and the damon_select_ops() is for paddr
ops setup.

But, damon_modules_new_paddr_ctx_target() calls damon_new_target() after that.
It could theoretically fail due to an allocation failure.

> it calls damon_destroy_ctx(ctx) to free the context. However, the
> global static ctx pointer is not set to NULL.
> 
> Since DAMON_LRU_SORT is a built-in module, its module parameters are exposed
> in sysfs early during boot. If a user reads the sysfs parameters after an init
> failure, the dangling ctx pointer would pass the check here, leading to
> damon_is_running() dereferencing it:
> 
> mm/damon/core.c:damon_is_running()
>     mutex_lock(&ctx->kdamond_lock);

Makes sense.  I will work on this.  Not a blocker of this patch though.

> 
> [ ... ]
> 
> > @@ -301,15 +291,57 @@ static int damon_lru_sort_enabled_store(const char *val,
> >  	return 0;
> >  }
> >  
> > +static int damon_lru_sort_enabled_load(char *buffer,
> > +		const struct kernel_param *kp)
> > +{
> > +	return sprintf(buffer, "%c\n", damon_lru_sort_enabled() ? 'Y' : 'N');
> > +}
> > +
> >  static const struct kernel_param_ops enabled_param_ops = {
> >  	.set = damon_lru_sort_enabled_store,
> > -	.get = param_get_bool,
> > +	.get = damon_lru_sort_enabled_load,
> >  };
> 
> Does this change actually fix the restart issue, or does it leave the state
> machine deadlocked?
> 
> If kdamond stops unintentionally, the internal enabled and last_enabled
> variables remain true. When the user tries to restart it by writing 'Y' to the
> sysfs enabled parameter, the underlying boolean value remains true. The worker
> skips the restart because last_enabled == now_enabled.
> 
> If the user tries to reset the state by writing 'N',
> damon_lru_sort_turn(false) calls damon_stop() which returns an error because
> the daemon is already stopped. Because of this error, the worker executes
> enabled = last_enabled, reverting enabled back to true:
> 
> mm/damon/lru_sort.c:damon_lru_sort_timer_fn()
>     if (!damon_lru_sort_turn(now_enabled))
>         last_enabled = now_enabled;
>     else
>         enabled = last_enabled;
> 
> It seems the subsystem still cannot be restarted without a reboot.

Correct, damon_lru_sort_enabled_store() part upstream change is not properly
ported.  I will post a new version.


Thanks,
SJ

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/2026051243-crevice-spool-75d8@gregkh?part=2