Re: [RFC PATCH v3 2/4] samples/damon/prcl: handle damon_start() failure

[SeongJae Park <sj@kernel.org>]

On Wed, 10 Jun 2026 01:29:48 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Loading the module dynamically with the `enabled=Y` parameter causes a permanent DAMON thread and context leak.
> - [Medium] The DAMON context, targets, and schemes are leaked if `damon_call()` fails in `damon_sample_prcl_start()`.
> --
> 
> commit 66049e90074b43852fd5c6e982d320d3e8718fb4
> Author: SeongJae Park <sj@kernel.org>
> 
> samples/damon/prcl: handle damon_start() failure
> 
> This commit fixes a memory leak in damon_sample_prcl_start() by destroying
> the Damon context if damon_start() fails. This addresses an issue where
> the context is leaked when the module fails to start in exclusive mode.
> 
> Link: https://lore.kernel.org/20260609145814.70163-1-sj@kernel.org
> 
> > diff --git a/samples/damon/prcl.c b/samples/damon/prcl.c
> > index b7c50f2656ce7..0db2598946911 100644
> > --- a/samples/damon/prcl.c
> > +++ b/samples/damon/prcl.c
> > @@ -106,8 +106,10 @@ static int damon_sample_prcl_start(void)
> >  	damon_set_schemes(ctx, &scheme, 1);
> >  
> >  	err = damon_start(&ctx, 1, true);
> > -	if (err)
> > +	if (err) {
> > +		damon_destroy_ctx(ctx);
> >  		return err;
> > +	}
> >  
> >  	repeat_call_control.data = ctx;
> >  	return damon_call(ctx, &repeat_call_control);
> 
> [Severity: Medium]
> This is a pre-existing issue, but does this function also leak the Damon
> context, targets, and schemes if damon_call() fails?
> 
> If damon_start() succeeds, it spawns the kdamond thread. If the newly spawned
> thread terminates early, for example due to a memory allocation failure for
> regions_score_histogram in kdamond_fn(), it sets call_controls_obsolete.
> 
> If this happens before damon_call() acquires the call_controls_lock:
> 
> mm/damon/core.c:damon_call() {
>     ...
> 	mutex_lock(&ctx->call_controls_lock);
> 	if (ctx->call_controls_obsolete) {
> 		mutex_unlock(&ctx->call_controls_lock);
> 		return -ECANCELED;
> 	}
>     ...
> }
> 
> Because damon_sample_prcl_start() returns this error directly to the caller
> without cleanup, the caller receives the error and disables the module. This
> permanently bypasses damon_sample_prcl_stop(), meaning damon_destroy_ctx()
> is never executed.

Good catch, I will fix this with another patch.

> 
> [Severity: High]
> This is also a pre-existing issue, but does loading the module dynamically
> with the enabled=Y parameter cause a permanent Damon thread and context leak?
> 
> During module load, the kernel parses parameters and invokes the module's
> parameter callback before executing the init function. If enabled=Y is
> passed, the callback successfully calls damon_sample_prcl_start(), which
> allocates the context and starts the kdamond thread in exclusive mode.
> 
> Then, do_init_module() executes damon_sample_prcl_init():
> 
> samples/damon/prcl.c:damon_sample_prcl_init() {
>     ...
> 	if (enabled) {
> 		err = damon_sample_prcl_start();
> 		if (err)
> 			return err;
> 	}
>     ...
> }
> 
> Because the enabled flag is still true, damon_sample_prcl_init() erroneously
> calls damon_sample_prcl_start() a second time. This unconditionally allocates
> a new ctx, overwriting the global ctx pointer and losing the reference to the
> original context.
> 
> The second damon_start() call fails with -EBUSY because the first thread is
> already running in exclusive mode. The original kdamond thread is left
> running forever and its context pointer is permanently lost, locking out any
> other Damon instances on the system until reboot.

prcl cannot dynamiclly loaded, so this issue cannot happen.


Thanks,
SJ

[...]