Re: [RFC PATCH] mm/damon/stat: detect and use fresh enabled status

[SeongJae Park <sj@kernel.org>]

On Thu, 16 Apr 2026 18:51:58 +0000 sashiko-bot@kernel.org wrote:

> Sashiko AI review found 1 potential issue(s):
> - [Medium] Writing 'N' to the `enabled` parameter after kdamond has stopped unexpectedly bypasses `damon_stat_stop()`, leaking `damon_stat_context`.

This is not a real problem.  Read below for more details.

> --
> 
> commit 1171ea4d01e310ec9987bf3cb2d19f8bf89b60cb
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/stat: detect and use fresh enabled status
> 
> This commit changes the enabled parameter's get callback to report the
> actual running status of kdamond instead of the static enabled variable.
> It introduces damon_stat_enabled() to dynamically check if the context is
> running.
> 
> > diff --git a/mm/damon/stat.c b/mm/damon/stat.c
> > index 99ba346f9e325..3951b762cbddf 100644
> > --- a/mm/damon/stat.c
> > +++ b/mm/damon/stat.c
> [ ... ]
> > @@ -273,17 +276,23 @@ static void damon_stat_stop(void)
> >  	damon_stat_context = NULL;
> >  }
> >  
> > +static bool damon_stat_enabled(void)
> > +{
> > +	if (!damon_stat_context)
> > +		return false;
> > +	return damon_is_running(damon_stat_context);
> > +}
> > +
> >  static int damon_stat_enabled_store(
> >  		const char *val, const struct kernel_param *kp)
> >  {
> > -	bool is_enabled = enabled;
> >  	int err;
> >  
> >  	err = kstrtobool(val, &enabled);
> >  	if (err)
> >  		return err;
> >  
> > -	if (is_enabled = enabled)
> > +	if (damon_stat_enabled() = enabled)
> >  		return 0;
> 
> If kdamond stops unexpectedly (for example, due to an allocation failure in
> kdamond_fn), damon_is_running() and consequently damon_stat_enabled() will
> return false.
> 
> If a user then explicitly disables the module by writing 'N' to the enabled
> parameter, kstrtobool() will update the global enabled variable to false.
> 
> Could this cause the check above to evaluate to true (false = false) and
> return 0 early?

Yes, that can happen.

> 
> >  
> >  	if (!damon_initialized())
> > @@ -293,16 +302,17 @@ static int damon_stat_enabled_store(
> >  		 */
> >  		return 0;
> >  
> > -	if (enabled) {
> > -		err = damon_stat_start();
> > -		if (err)
> > -			enabled = false;
> > -		return err;
> > -	}
> > +	if (enabled)
> > +		return damon_stat_start();
> >  	damon_stat_stop();
> 
> If the early return is taken, damon_stat_stop() is bypassed. Does this leak
> damon_stat_context since damon_destroy_ctx() won't be called?

That's not leaked because we still keep the pointer and free that when
damon_stat_start() is called next time.

> 
> >  	return 0;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260416143857.76146-1-sj@kernel.org?part=1


Thanks,
SJ

[...]