Re: [PATCH v3 3/3] mm/damon/stat: detect and use fresh enabled value

[SeongJae Park <sj@kernel.org>]

On Sun, 19 Apr 2026 17:51:20 +0000 sashiko-bot@kernel.org wrote:

> Sashiko AI review found 2 potential issue(s):
> - [Critical] Passing `enabled=Y` during module load triggers a double-start, failing initialization and causing a Use-After-Free kernel panic.
> - [High] Resource leak of `damon_stat_context` when explicitly disabling the module after an internal kdamond crash.

Both are wrong.  Please read below for more details.

> --
> 
> commit 16eae8a1bf380cdb61d4dd6b62187dd09f8ce859
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/stat: detect and use fresh enabled value
> 
> This commit updates the DAMON_STAT module to dynamically detect the running
> status of its kdamond thread rather than relying on a cached parameter
> value.
> 
> Link: https://lore.kernel.org/20260416040602.88665-1-sj@kernel.org
> 
> > 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 terminates asynchronously (for example, due to an allocation
> failure), damon_stat_enabled() will return false.
> 
> If a user subsequently disables the module by writing 'N' to the enabled
> parameter, this condition (false == false) evaluates to true and the
> function returns 0.
> 
> Does this skip the call to damon_stat_stop() and leak the
> damon_stat_context since damon_destroy_ctx() won't be called?

I previously explained [1] it is not leaked.

> 
> >  
> >  	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();
> 
> During module load via "insmod damon_stat.ko enabled=Y", parse_args() calls
> this store function. Because damon_stat_enabled() dynamically returns false
> at this point, the code proceeds to call damon_stat_start() to start the
> kdamond thread.
> 
> Shortly after, the module initialization function damon_stat_init() runs,
> sees enabled == true, and calls damon_stat_start() again. This second call
> returns -EAGAIN because the context is already running.
> 
> The module initialization then fails, causing the kernel to unload the
> module and free its memory.
> 
> Since the kdamond thread is still actively running in the background, can
> this cause a Use-After-Free when the thread accesses the freed module
> memory?
> 
> While this issue previously existed if CONFIG_DAMON_STAT_ENABLED_DEFAULT=n,
> does this patch introduce the regression for the default configuration as
> well?

DAMON_STAT is a builtin module.  Users cannot load it dynamically, and
therefore the above scenario cannot happen.

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


Thanks,
SJ

[...]