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

[sashiko-bot@kernel.org]

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.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610011420.3018-1-sj@kernel.org?part=2