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

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] Writing to the `enabled` sysfs parameter can cause a NULL pointer dereference if `damon_reclaim_init()` failed during boot.
--

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

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

This commit fixes an issue where the 'enabled' and 'kdamond_pid' parameter
values can become stale if the kdamond thread stops unintentionally. It
updates the parameter accessors to dynamically detect and return the fresh
status from the DAMON context.

Cc: <stable@vger.kernel.org> # 5.19.x

> 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);
>  	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 result in a NULL pointer dereference if damon_reclaim_init() fails
during boot?

This issue existed previously and appears to still be present. If the init
function fails (for example, due to a memory allocation failure), the global
ctx pointer remains NULL.

If a user later writes 'Y' to the enabled sysfs parameter, the following
sequence seems possible:

damon_reclaim_enabled_store()
    damon_reclaim_enabled() returns false because ctx is NULL
    damon_initialized() returns true because the core framework initialized
    damon_reclaim_turn(true)
        damon_reclaim_apply_parameters()
            damon_commit_ctx(ctx, param_ctx)

And then inside damon_commit_ctx():

int damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src)
{
    ...
    dst->maybe_corrupted = true;
    ...
}

Since dst is the global ctx pointer, could this crash the system if a
local user writes to the sysfs parameter when ctx was never allocated?

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

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