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

[sashiko-bot@kernel.org]

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), 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);

[ ... ]

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

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