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

[SeongJae Park <sj@kernel.org>]

On Sat, 18 Apr 2026 22:57:57 +0000 sashiko-bot@kernel.org wrote:

TL; DR: no blocker for this patch.

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

Good catch, but not the fault of this patch.  I will separately work on it.

> - [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.

Sashiko-claiming issue cannot happen.  Read below for more details.

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

Sysfs writes are protected by kernel_param_lock, so such concurrent
user inputs cannot happen.

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

Good finding, but the bug was there even before this patch, so no blocker of
this patch.  I will separately work on the bug.


Thanks,
SJ

[...]