Re: [RFC PATCH v1.1 06/16] mm/damon/paddr: respect return_max_wsum

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `probe_hits` array in `struct damon_region` is of type `unsigned char`, causing silent wrap-around if the number of sampling intervals in an aggregation interval (`aggr_samples`) exceeds 255.
--

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

mm/damon/paddr: respect return_max_wsum

apply_probes() ops implementation in DAMON_PADDR is ignoring
return_max_wsum.  Respect it.

> diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> index 3eba3564a31fc..b85f88a7a38f4 100644
> --- a/mm/damon/paddr.c
> +++ b/mm/damon/paddr.c

[ ... ]

> @@ -193,9 +193,12 @@ static unsigned int damon_pa_apply_probes(struct damon_ctx *ctx,
>  			}
>  			if (folio)
>  				folio_put(folio);
> +			if (return_max_wsum)
> +				max_wsum = max(damon_probe_hits_wsum(r, false,
> +							ctx), max_wsum);
>  		}
>  	}
> -	return 0;
> +	return max_wsum;
>  }

[Severity: High]
This is a pre-existing issue, but I noticed something about the probe_hits
array while reviewing this area. Does this code silently wrap around if the
number of samples in an aggregation interval exceeds 255?

Looking at the damon_region structure in include/linux/damon.h, probe_hits
is defined as an unsigned char array:

include/linux/damon.h:struct damon_region {
    ...
    unsigned char probe_hits[DAMON_MAX_PROBES];
    ...
};

In mm/damon/paddr.c:damon_pa_apply_probes(), this value is incremented on
every sampling interval without bounds checking:

mm/damon/paddr.c:damon_pa_apply_probes() {
    ...
    damon_for_each_probe(p, ctx) {
        if (damon_pa_filter_pass(pa, folio, p))
            r->probe_hits[i]++;
        i++;
    }
    ...
}

If a user configures the sysfs settings such that the aggregation interval
is significantly larger than the sample interval (for example, a 1ms
sample interval and a 1s aggregation interval yields 1000 samples), will
r->probe_hits[i] wrap around and start over from zero?

If so, it appears this might break DAMON region merging and monitoring
statistics by operating on incorrect, wrapped-around numbers.

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