Re: [PATCH] mm/damon/core: handle region split failure in apply_min_nr_regions()

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

Pre-existing issues:
- [High] Incomplete bug fix in damon_apply_min_nr_regions() leads to O(N^2) allocator thrashing on allocation failure.
- [High] Silent failure of damon_split_region_at() bypasses DAMON quotas and bounds checks.
--

--- Patch [1]: [PATCH] mm/damon/core: handle region split failure in apply_min_nr_regions() ---
commit 51e7e9012c52dde83bac4be44f0bbfbdbe01572d
Author: SJ Park <[email protected]>
Subject: mm/damon/core: handle region split failure in apply_min_nr_regions()

damon_apply_min_nr_regions() repeatedly split each region until its size
becomes small enough to meet the user-defined low limit of the number of
regions. The loop assumes the split operation (damon_split_region_at())
will always succeed and create the new region.

Link: https://lore.kernel.org/[email protected]

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 20d267c615faf..3d829e0ad63b5 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1913,6 +1913,9 @@ static unsigned long damon_apply_min_nr_regions(struct damon_ctx *ctx)
>  		damon_for_each_region_safe(r, next, t) {
>  			while (damon_sz_region(r) > max_region_sz) {
>  				damon_split_region_at(t, r, max_region_sz);
> +				/* split might failed */
> +				if (r == damon_last_region(t))
> +					break;

[Severity: High]
This isn't a bug introduced by this patch, but does this fix leave an
O(N^2) allocator thrashing loop when an allocation fails on a non-last
region?

If a region fails to split due to memory pressure and is not the last
region, the inner loop unconditionally advances r = damon_next_region(r).
This moves to the next pre-existing region instead of a newly created
one, and the inner loop will attempt to split this next region, continuing
to the end of the list.

Then, the outer damon_for_each_region_safe() loop advances to its
pre-saved next pointer and repeats the entire process.

Could this severely thrash the page allocator with redundant
kmem_cache_alloc() calls and cause RCU stalls or lockups during an
OOM condition?

[Severity: High]
This is also a pre-existing issue and was not introduced by this patch, but
since we are handling split failures here, could the silent failure of
damon_split_region_at() bypass DAMON quotas in other callers?

Because damon_split_region_at() returns void, callers like
damos_apply_scheme() cannot detect if the split failed:

mm/damon/core.c:damos_apply_scheme() {
...
			damon_split_region_at(t, r, sz);
		}
		if (damos_core_filter_out(c, t, r, s))
			return;
...
		sz_applied = c->ops.apply_scheme(c, t, r, s,
				&sz_ops_filter_passed);
...
		damos_charge_quota(quota, sz, sz_applied);
}

If the split fails, the region remains oversized and the scheme is
erroneously applied to the entire original region.

The quota charging logic then only charges the intended small size (sz),
which permanently hides the limit bypass.

Could this cause DAMON schemes to violate their strict resource quota
limits and apply actions to much larger memory areas than permitted?

>  				r = damon_next_region(r);
>  			}
>  		}

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