Re: [PATCH 6.18.y] mm/damon/reclaim: 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] Writing to the `enabled` sysfs parameter triggers a NULL pointer dereference if the module's initialization failed.
- [Low] The commit introduces a call to an undefined function `damon_kdamond_pid()`, causing a build failure.
--

commit 45d783d4b3fd23532d7c6b975b70fda3e5324f1b
Author: SeongJae Park <sj@kernel.org>
mm/damon/reclaim: detect and use fresh enabled and kdamond_pid values

This commit modifies the DAMON reclaim module to dynamically detect and use
the fresh running status of kdamond when parameters are requested. This
prevents stale cached statuses from being shown to users when kdamond stops
unexpectedly.

Link: https://lore.kernel.org/20260419161003.79176-1-sj@kernel.org
Link: https://lore.kernel.org/20260419161003.79176-2-sj@kernel.org

> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> index 7ba3d0f9a19ac..9446e7a1b476f 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c

[ ... ]

> @@ -334,42 +320,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);
> +}

If module initialization fails during boot, for example due to an allocation
failure in damon_modules_new_paddr_ctx_target(), the global ctx pointer will
remain NULL. Because damon_reclaim is built-in, its sysfs parameters are
still exposed.

If a user subsequently writes to the enabled parameter,
damon_reclaim_enabled_store() will see that damon_initialized() is true
because the global cache is initialized, and it will call
damon_reclaim_turn().

This unconditionally passes the NULL ctx pointer to either damon_stop() or
damon_reclaim_apply_parameters(), which eventually calls damon_commit_ctx().

Could this cause a NULL pointer dereference when accessing the uninitialized
ctx pointer?

[ ... ]

> +static int damon_reclaim_kdamond_pid_load(char *buffer,
> +		const struct kernel_param *kp)
> +{
> +	int kdamond_pid = -1;
> +
> +	if (ctx) {
> +		kdamond_pid = damon_kdamond_pid(ctx);

Is the function damon_kdamond_pid() defined anywhere in the tree?

It appears this function is called here but might be missing, which would
cause an implicit declaration and linker error. Could a prerequisite patch be
missing from this cherry-pick?

> +		if (kdamond_pid < 0)
> +			kdamond_pid = -1;
> +	}
> +	return sprintf(buffer, "%d\n", kdamond_pid);
> +}

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