Re: [PATCH v2] samples/damon/mtier: fail early if address range parameters are invalid

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Memory leak of `damon_ctx` structures and potential orphaned `kdamond` threads when `damon_start()` fails.
- [High] Incomplete fix allowing integer overflow and resulting in `WARN_ONCE()` and massive region size.
--

commit 567b4e8a4b1811419a6203e400b62d93a47bfe84
Author: Zenghui Yu <yuzenghui@huawei.com>

samples/damon/mtier: fail early if address range parameters are invalid

This commit adds an early check in damon_sample_mtier_build_ctx() to ensure
the address range parameters are valid. This is intended to prevent a
WARN_ONCE() that occurs when the node address bounds are not properly
initialized before the module is enabled.

> diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> index eb1143de8df17..3785b0c7ffb17 100644
> --- a/samples/damon/mtier.c
> +++ b/samples/damon/mtier.c
> @@ -120,6 +120,9 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
>  		addr.end = promote ? node1_end_addr : node0_end_addr;
>  	}
>  
> +	if (addr.start >= addr.end)
> +		goto free_out;
> +

[Severity: High]
This isn't a bug introduced by this patch, but does this incomplete fix still
allow an integer overflow?

If addr.end is set to ULONG_MAX via sysfs and addr.start is a valid address,
this check passes. Later, damon_set_regions() calls ALIGN():

mm/damon/core.c:damon_set_regions() {
    ...
    ranges[i].end = ALIGN(ranges[i].end, min_region_sz);
    ...
}

The ALIGN macro will add min_region_sz - 1 to ULONG_MAX, causing an integer
overflow that wraps the value to 0. This results in calling
damon_new_region(start, 0), which triggers the exact WARN_ONCE() this
patch intended to fix.

Additionally, could this cause damon_sz_region() to return a massive value,
breaking the logic in damon_apply_min_nr_regions() and causing excessive
allocations?

>  	range.start = addr.start;
>  	range.end = addr.end;

[Severity: High]
This is a pre-existing issue, but does this code leak the context structures
and orphan the threads if damon_start() fails?

samples/damon/mtier.c:damon_sample_mtier_start() {
    ...
	ctxs[1] = ctx;
	return damon_start(ctxs, 2, true);
}

If damon_start() fails (e.g., returning -EBUSY or -ENOMEM), it returns the
error directly without cleaning up the dynamically allocated contexts in
ctxs[0] and ctxs[1].

The caller damon_sample_mtier_enable_store() sets enabled = false on error.
Subsequent attempts to enable the module will allocate new contexts and
overwrite the ctxs array, permanently leaking the previous contexts.

If damon_start() successfully starts the first context but fails on the
second, does the first context's kdamond thread remain running as an
orphaned thread?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609064653.1829-1-yuzenghui@huawei.com?part=1