Re: [PATCH 6.18.y] mm/damon/reclaim: detect and use fresh enabled and kdamond_pid values

[SeongJae Park <sj@kernel.org>]

On Thu, 14 May 2026 02:02:41 +0000 sashiko-bot@kernel.org wrote:

> 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?

Sashiko reported [1] same thing before, and I'm working on it.

> 
> [ ... ]
> 
> > +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?

I informed this issue and where the dependent patch can be found, on the
commentary area of the patch.

[1] https://lore.kernel.org/all/20260419014800.877-1-sj@kernel.org/


Thanks,
SJ

> 
> > +		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