Re: [RFC PATCH v1.1 07/16] mm/damon/core: extend merge function to work with probe hits

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] API Misuse / Silent Overflow: Using `abs()` on the difference of two large unsigned integers causes incorrect results.
- [High] Integer Overflow: `damon_probe_hits_wsum()` wraps around the 32-bit boundary when large weights are used.
--

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

mm/damon/core: extend merge function to work with probe hits

When probe weights are set, this commit extends damon_merge_regions_of() to
work with the weighted sum of probe hits. It also introduces a placeholder
detection function to allow safe incremental changes.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 42f27849af722..7aff741d43bd4 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -3230,6 +3235,16 @@ static void damon_merge_two_regions(struct damon_target *t,
>  	damon_destroy_region(r, t);
>  }
>  
> +static unsigned int damon_merge_score(struct damon_region *r, bool last,
> +		struct damon_ctx *ctx, bool use_probe_hits)
> +{
> +	if (use_probe_hits)
> +		return damon_probe_hits_wsum(r, last, ctx);
                 ^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this code wrap around the 32-bit boundary when large weights are used?

Looking at damon_probe_hits_wsum(), it accumulates probe_hits[i] * probe->weight
into a 32-bit unsigned int sum:

mm/damon/core.c:damon_probe_hits_wsum() {
    ...
    unsigned int sum = 0;
    ...
        if (last)
            sum += r->last_probe_hits[i++] * probe->weight;
        else
            sum += r->probe_hits[i++] * probe->weight;
    ...
    return sum;
}

If a user configures a large weight (e.g., > 16,843,000) for a probe via
sysfs, this could wrap around (e.g., 255 hits * 40,000,000 weight = 10.2
billion, which wraps to ~1.6 billion).

Could this break the scoring mechanism, causing damon_merge_regions_of() to
merge active and inactive regions incorrectly?

> +	if (last)
> +		return r->last_nr_accesses;
> +	return r->nr_accesses;
> +}
> +

[ ... ]

> @@ -3238,24 +3253,37 @@ static void damon_merge_regions_of(struct damon_target *t, unsigned int thres,
> -				   unsigned long sz_limit)
> +		unsigned long sz_limit, struct damon_ctx *ctx)
>  {
>  	struct damon_region *r, *prev = NULL, *next;
> +	bool use_probe_hits = damon_has_probe_weights(ctx);
>  
>  	damon_for_each_region_safe(r, next, t) {
> -		if (abs(r->nr_accesses - r->last_nr_accesses) > thres)
> +		unsigned int score, last_score;
> +
> +		score = damon_merge_score(r, false, ctx, use_probe_hits);
> +		last_score = damon_merge_score(r, true, ctx, use_probe_hits);
> +
> +		if (abs(score - last_score) > thres)
                   ^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Can this call to abs() cause incorrect results due to silent overflow?

Since score and last_score are unsigned integers, abs(score - last_score)
casts the unsigned difference to a signed int.

If the actual difference exceeds the maximum signed integer value, it wraps
to a negative value, which abs() then negates into a smaller positive value.

Could this falsely small difference cause the threshold check > thres to fail
incorrectly, allowing vastly different regions to be merged and affecting the
accuracy of the monitor?

>  			r->age = 0;
> -		else if ((r->nr_accesses == 0) != (r->last_nr_accesses == 0))
> +		else if ((score == 0) != (last_score == 0))
>  			r->age = 0;
>  		else
>  			r->age++;
>  
> -		if (prev && prev->ar.end == r->ar.start &&
> -		    abs(prev->nr_accesses - r->nr_accesses) <= thres &&
> -		    damon_sz_region(prev) + damon_sz_region(r) <= sz_limit)
> -			damon_merge_two_regions(t, prev, r);
> -		else
> -			prev = r;
> +		if (!prev)
> +			goto set_prev_continue;
> +		if (prev->ar.end != r->ar.start)
> +			goto set_prev_continue;
> +		if (abs(damon_merge_score(prev, false, ctx, use_probe_hits) -
> +					score) > thres)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this second use of abs() on the unsigned difference of the merge scores
suffer from the exact same signed-integer overflow vulnerability?

> +			goto set_prev_continue;
> +		if (damon_sz_region(prev) + damon_sz_region(r) > sz_limit)
> +			goto set_prev_continue;
> +		damon_merge_two_regions(t, prev, r);
> +		continue;
> +set_prev_continue:
> +		prev = r;
>  	}
>  }

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