Re: [RFC PATCH v2.1 1/3] mm/damon/reclaim: detect and use fresh enabled and kdamond_pid values

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Missing context initialization check in the sysfs store handler can trigger a NULL pointer dereference if module initialization fails.
- [Medium] The `damon_reclaim_enabled_store` function parses user input directly into the global `enabled` variable before using it for control flow decisions, causing a Time-Of-Check to Time-Of-Use (TOCTOU) race condition.
--

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

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

DAMON_RECLAIM updates the enabled and kdamond_pid parameter values dynamically
to prevent stale values when the daemon is stopped unintentionally. This
ensures users see the fresh running status and can restart the daemon when
needed.

> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> index 86da147786583..fe7fce26cf6ce 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c

[ ... ]

> @@ -332,42 +316,83 @@ module_param_cb(addr_unit, &addr_unit_param_ops, &addr_unit, 0600);
>  MODULE_PARM_DESC(addr_unit,
>  	"Scale factor for DAMON_RECLAIM to ops address conversion (default: 1)");
>  
> +static bool damon_reclaim_enabled(void)
> +{
> +	if (!ctx)
> +		return false;
> +	return damon_is_running(ctx);
> +}
> +
>  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);

By parsing user input directly into the global enabled variable, does this
introduce a race condition?

If multiple threads write to the sysfs enabled file concurrently, thread A's
parsed input could be overwritten by thread B before thread A evaluates it
in damon_reclaim_turn(enabled).

Could this cause thread A to execute the opposite action of what was
requested while still returning success to the user?

>  	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 (!damon_initialized())
> -		goto set_param_out;
> +		return 0;
>  
> -	err = damon_reclaim_turn(enable);
> -	if (err)
> -		return err;
> +	return damon_reclaim_turn(enabled);

Can this trigger a null pointer dereference if the module initialization
fails?

If damon_reclaim is built into the kernel and damon_reclaim_init() fails
(for example, due to an allocation failure), the global ctx pointer remains
null. Because it is a built-in module, the sysfs parameters remain exposed.

When a user writes 'Y' to the enabled file, damon_initialized() will pass
because the core DAMON subsystem initialized successfully during boot. The
code then calls damon_reclaim_turn(true) which passes the null ctx down to
damon_commit_ctx(ctx, param_ctx).

Should there be an explicit if (!ctx) validation before attempting to start
the reclaim process?

> -set_param_out:
> -	enabled = enable;
> -	return err;
> +}

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