Re: [RFC PATCH v2 2/3] mm/damon/lru_sort: detect and use fresh enabled and kdamond_pid values

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [Critical] Missing `.set` callback in `kdamond_pid_param_ops` will cause a kernel panic if the parameter is passed at boot or via modprobe.
- [High] Loading the module with `enabled=Y` causes a NULL pointer dereference on `ctx`.
- [High] Concurrent calls to `damon_lru_sort_turn()` lack locking, leading to list corruption and kernel panics.
--

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

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

This commit updates DAMON_LRU_SORT to dynamically check the running status
of kdamond instead of relying on cached parameter values. This ensures the
status correctly reflects when kdamond stops due to events other than
explicit user requests.

> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 554559d729760..d284cb336b662 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c
[ ... ]
> @@ -386,12 +377,8 @@ static int damon_lru_sort_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);

Could concurrent calls to this function lead to list corruption? 

If a user writes 'Y' to the enabled parameter via sysfs at the same
time damon_lru_sort_init() calls damon_lru_sort_turn(true) during module
initialization, damon_commit_ctx() might concurrently modify ctx->schemes
and ctx->targets without internal locking.

[ ... ]
> @@ -430,10 +414,16 @@ module_param_cb(addr_unit, &addr_unit_param_ops, &addr_unit, 0600);
>  MODULE_PARM_DESC(addr_unit,
>  	"Scale factor for DAMON_LRU_SORT to ops address conversion (default: 1)");
>  
> +static bool damon_lru_sort_enabled(void)
> +{
> +	if (!ctx)
> +		return false;
> +	return damon_is_running(ctx);
> +}
> +
>  static int damon_lru_sort_enabled_store(const char *val,
>  		const struct kernel_param *kp)
>  {
> -	bool is_enabled = enabled;
>  	bool enable;
>  	int err;

Is it possible to hit a NULL pointer dereference on ctx when loading the
module with enabled=Y? 

If the DAMON core is already initialized (e.g. built-in), damon_initialized()
would return true, bypassing the early exit in this function. This could
lead to calling damon_lru_sort_turn(true) before damon_lru_sort_init() has
run, causing damon_commit_ctx() to operate on a NULL ctx pointer.

[ ... ]
> +static const struct kernel_param_ops kdamond_pid_param_ops = {
> +	.get = damon_lru_sort_kdamond_pid_load,
> +};
> +
> +/*
> + * PID of the DAMON thread
> + *
> + * If DAMON_LRU_SORT is enabled, this becomes the PID of the worker thread.
> + * Else, -1.
> + */
> +module_param_cb(kdamond_pid, &kdamond_pid_param_ops, NULL, 0400);

Does this cause a NULL pointer dereference if the parameter is passed at
boot or via modprobe?

The kernel parameter parsing code in parse_one() doesn't check if .set is
NULL before invoking it:

    err = params[i].ops->set(val, &params[i]);

A dummy .set function returning an error might be needed to prevent a NULL
pointer dereference when parsing command line arguments.

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