Re: [PATCH 03/11] mm/damon/core: introduce failed region quota charge ratio

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Pathological region fragmentation causing catastrophic CPU overhead when actions fail.
- [Medium] Silent 32-bit integer overflow in `mult_frac()` for unbounded charge ratio parameters.
--

commit 4805d0ecef06fc0ab3681758cf2d692b37ed9b70
Author: SeongJae Park <sj@kernel.org>

mm/damon/core: introduce failed region quota charge ratio

This commit introduces a mechanism to charge action-failed memory at a
different ratio than action-successful memory. It allows API callers to
specify the ratio flexibly by setting a numerator and denominator for the
failed region charge rate.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 19ec9919b5ca..87cd4f0d14f3 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -2046,6 +2048,23 @@ static void damos_walk_cancel(struct damon_ctx *ctx)
>  	mutex_unlock(&ctx->walk_control_lock);
>  }
>  
> +static void damos_charge_quota(struct damos_quota *quota,
> +		unsigned long sz_region, unsigned long sz_applied)
> +{
> +	/*
> +	 * sz_applied could be bigger than sz_region, depending on ops
> +	 * implementation of the action, e.g., damos_pa_pageout().  Charge only
> +	 * the region size in the case.
> +	 */
> +	if (!quota->fail_charge_denom || sz_applied > sz_region)
> +		quota->charged_sz += sz_region;
> +	else
> +		quota->charged_sz += sz_applied + mult_frac(
> +				(sz_region - sz_applied),
> +				quota->fail_charge_num,
> +				quota->fail_charge_denom);

Can this silently overflow on 32-bit architectures?

The mult_frac(x, n, d) macro evaluates (x / d) * n + (x % d) * n / d. Since
sz_region is unsigned long, on 32-bit systems the intermediate remainder
(x % d) is a 32-bit integer. The variables fail_charge_num and
fail_charge_denom are declared as unsigned int and could be set to
arbitrarily large values (up to UINT_MAX) without bounds checking. 

If large inputs are provided, does the (x % d) * n multiplication silently
wrap around before division, causing an incorrectly small charged amount
and undermining the quota enforcement?

> +}
> +

[ ... ]

> @@ -2116,7 +2135,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
>  		ktime_get_coarse_ts64(&end);
>  		quota->total_charged_ns += timespec64_to_ns(&end) -
>  			timespec64_to_ns(&begin);
> -		quota->charged_sz += sz;
> +		damos_charge_quota(quota, sz, sz_applied);

Could this cause pathological region fragmentation and massive CPU overhead
if the action fails?

Earlier in damos_apply_scheme(), if the remaining quota is small, the
region is split to fit:

	if (damos_quota_is_set(quota) &&
			quota->charged_sz + sz > quota->esz) {
		sz = ALIGN_DOWN(quota->esz - quota->charged_sz,
				c->min_region_sz);
		if (!sz)
			goto update_stat;
		damon_split_region_at(t, r, sz);
	}

If the action then fails, sz_applied could be 0. When damos_charge_quota()
is called, if fail_charge_num is also small or 0, charged_sz doesn't
increase by sz. This leaves the remaining quota unchanged. 

In the next iteration, the remainder of the previously split region is
processed. Since the remaining quota is exactly the same, it calculates the
same sz and splits the region again. This process repeats, dividing the
entire remaining address space into tiny min_region_sz chunks, potentially
triggering millions of loops and pegging the kdamond thread at 100% CPU.

>  		if (damos_quota_is_full(quota, c->min_region_sz)) {
>  			quota->charge_target_from = t;
>  			quota->charge_addr_from = r->ar.end + 1;

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