Re: [PATCH 1/2] mm/damon/core: cover discrete System RAM areas with per-range regions

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Time-of-check to time-of-use (TOCTOU) race condition in `damon_find_system_rams()` leads to a heap out-of-bounds write during concurrent memory hotplug.
- [High] DAMON initialization fails with `-EINVAL` on systems with sub-page System RAM gaps due to alignment overlap in `damon_set_regions()`.
--

--- Patch [1]: [PATCH 1/2] mm/damon/core: cover discrete System RAM areas with per-range regions ---
commit 20fb01bf6e8e4cefc6016dfc073fb77c95ca3511
Author: Jiayuan Chen <[email protected]>

mm/damon/core: cover discrete System RAM areas with per-range regions

This commit changes how DAMON sets default monitoring regions for System RAM.
Instead of a single region covering all RAM (and the holes between), it now
creates separate regions for each discrete System RAM area. This excludes
large memory holes from being monitored.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 644daf5a16560..365c43271c0c6 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> +static int damon_find_system_rams(struct damon_addr_range **ranges_out,
> +				  unsigned long addr_unit)
> +{
> +	struct damon_system_ram_range_walk_arg arg = { .addr_unit = addr_unit };
> +	struct damon_addr_range *ranges;
>  
> +	/* First pass: count the coalesced ranges. */
>  	walk_system_ram_res(0, -1, &arg, damon_system_ram_walk_fn);
> -	if (!arg.walked)
> -		return false;
> -	*start = damon_res_to_core_addr(arg.res.start, addr_unit);
> -	*end = damon_res_to_core_addr(arg.res.end + 1, addr_unit);
> -	if (*end <= *start)
> -		return false;
> -	return true;
> +	if (!arg.nr_ranges)
> +		return 0;
> +	ranges = kvmalloc_objs(*ranges, arg.nr_ranges, GFP_KERNEL);
> +	if (!ranges)
> +		return -ENOMEM;
> +	/* Second pass: fill in the coalesced ranges. */
> +	arg.ranges = ranges;
> +	arg.nr_ranges = 0;
> +	arg.has_prev = false;
> +	walk_system_ram_res(0, -1, &arg, damon_system_ram_walk_fn);

[Severity: High]
Can this race with memory hotplug events and cause an out-of-bounds write?

walk_system_ram_res() is called twice without holding memory hotplug locks.
If a new System RAM region is hotplugged between or during the second pass,
the number of visited regions could exceed the allocated buffer size.

If that happens, damon_system_ram_walk_fn() would write past the end of the
kvmalloc-allocated buffer:

mm/damon/core.c:damon_system_ram_walk_fn() {
	...
	if (a->ranges) {
		a->ranges[a->nr_ranges].start = start;
		a->ranges[a->nr_ranges].end = end;
	}
	a->nr_ranges++;
	...
}

> +	*ranges_out = ranges;
> +	return arg.nr_ranges;
>  }

[ ... ]

> @@ -3984,14 +4021,25 @@ int damon_set_region_system_rams_default(struct damon_target *t,
>  			unsigned long addr_unit, unsigned long min_region_sz)
>  {
>  	struct damon_addr_range addr_range;
> +	struct damon_addr_range *ranges;
> +	int nr_ranges, err;
>  
> -	if (!*start && !*end &&
> -		!damon_find_system_rams_range(start, end, addr_unit))
> -		return -EINVAL;
> +	if (*start || *end) {
> +		addr_range.start = *start;
> +		addr_range.end = *end;
> +		return damon_set_regions(t, &addr_range, 1, min_region_sz);
> +	}
>  
> -	addr_range.start = *start;
> -	addr_range.end = *end;
> -	return damon_set_regions(t, &addr_range, 1, min_region_sz);
> +	nr_ranges = damon_find_system_rams(&ranges, addr_unit);
> +	if (nr_ranges < 0)
> +		return nr_ranges;
> +	if (!nr_ranges)
> +		return -EINVAL;
> +	*start = ranges[0].start;
> +	*end = ranges[nr_ranges - 1].end;
> +	err = damon_set_regions(t, ranges, nr_ranges, min_region_sz);

[Severity: High]
Could this fail with -EINVAL during initialization on systems with sub-page
memory gaps?

If the system has System RAM fragmented by small non-RAM holes (like sub-page
BIOS or EFI reserved regions), damon_system_ram_walk_fn() will create separate
discrete regions.

When these regions are passed to damon_set_regions(), it aligns the boundaries
to min_region_sz (typically PAGE_SIZE):

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

If the gap between two RAM areas is smaller than PAGE_SIZE, the ALIGN of the
first region's end will evaluate to a higher address than the ALIGN_DOWN of
the second region's start. Does this cause the strictly-ascending overlap
check (last_end > start) to trigger and reject valid configurations?

> +	kvfree(ranges);
> +	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1