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

[SJ Park <sj@kernel.org>]

On Thu, 02 Jul 2026 21:19:54 +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] `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.

Yes, and we are working for this with a separate hotfix [1].

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

This is an intended behavior.

Yes, this might be confusing.  But why a sane user would do this?  Maybe we can
update documentation for reducing the confusion, but not necessarily a blocker
of this patch.

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


Thanks,
SJ

[...]