Re: [RFC PATCH v1.1 6/8] mm/damon: document region size validation in damon_set_regions()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `damon_set_regions()` creates duplicate and overlapping regions if input ranges share aligned boundaries, corrupting DAMON's internal state.
- [Medium] `damon_set_regions()` inconsistently bypasses size validation, silently accepting and re-aligning negative-sized (`start > end`) ranges instead of rejecting them.
--

commit ded29c13e7c58ad791814e55b8139da57477556b
Author: SJ Park <sj@kernel.org>

mm/damon: document region size validation in damon_set_regions()

The kernel doc comment of damon_region clearly specifies every region
should have positive size. But it is unclear who should verify it.
damon_set_regions() is the recommended DAMON core function for setting
regions from the callers, and has the verification.

> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 805e089ff4f2f..b3c0ac5bf9b88 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -49,7 +49,8 @@ struct damon_size_range {
>   * @list:		List head for siblings.
>   * @age:		Age of this region.
>   *
> - * For any use case, @ar should be non-zero positive size.
> + * For any use case, @ar should be non-zero positive size.  damon_set_regions()
> + * does the validation.

[Severity: High]
This is a pre-existing issue, but if callers rely on damon_set_regions() to
do the validation, can it create duplicate and overlapping regions when input
ranges share aligned boundaries?

If userspace passes multiple valid non-overlapping ranges (e.g., [0, 100] and
[100, 200]) that align to the same min_region_sz (e.g., 4096), and the target
is empty, the loop in mm/damon/core.c:damon_set_regions() blindly adds
aligned ranges without checking for previous alignments:

mm/damon/core.c:damon_set_regions() {
...
			r = damon_new_region(
					ALIGN_DOWN(ranges[i].start,
						min_region_sz),
					ALIGN(ranges[i].end, min_region_sz));
			if (!r)
				return -ENOMEM;
			damon_add_region(r, t);
...
}

When regions exist, the iterator is not reset per input range:

mm/damon/core.c:damon_set_regions() {
...
	r = damon_first_region(t);
	/* Add new regions or resize existing regions to fit in the ranges */
	for (i = 0; i < nr_ranges; i++) {
...
		/* Get the first/last regions intersecting with the range */
		damon_for_each_region_from(r, t) {
...
}

It advances past the first intersection, fails to find the same region for the
second range, and creates a duplicate [0, 4096] region, which corrupts
DAMON's internal lists.

[Severity: Medium]
This isn't a bug introduced by this patch, but does damon_set_regions()
actually fully validate the size?

The validation check in mm/damon/core.c:damon_set_regions() uses aligned
boundaries:

mm/damon/core.c:damon_set_regions() {
...
		if (ALIGN_DOWN(ranges[i].start, min_region_sz) >=
				ALIGN(ranges[i].end, min_region_sz))
			return -EINVAL;
...
}

If a user writes a reversed range (e.g., start=4000, end=1000) via sysfs,
since 0 >= 4096 evaluates to false, wouldn't the invalid input be silently
accepted and transformed into [0, 4096], bypassing the intended size
restrictions?

>   *
>   * @nr_accesses is reset to zero for every &damon_attrs->aggr_interval and be
>   * increased for every &damon_attrs->sample_interval if an access to the region

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