Re: [RFC PATCH v1.1 4/7] mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The KUnit test `damon_test_sample_count_to_bp` unconditionally fails on 32-bit architectures due to integer overflow in `aggr_interval` initialization.
- [Low] The commit message erroneously claims to remove an obsolete division-by-zero comment that is not present in the patch diff.

Pre-existing issues:
- [High] Arithmetic overflow in `damon_sample_bp_to_count()` and `damon_sample_count_to_bp()` on all architectures when `nr_samples_per_aggr` is large.
--

--- Patch [4]: [RFC PATCH v1.1 4/7] mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/ ---
commit 3660ea5f51f638ba678520c22cb275f27a0966b1
Author: SJ Park <sj@kernel.org>
mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/

This commit renames damon_nr_accesses_to_accesses_bp() to
damon_sample_count_to_bp() to better reflect its behavior. It also mentions
removing an obsolete division-by-zero comment.

>     Also remove the obsolete comment about division-by-zero.  Commit
>     35d4a3cf70a8 ("mm/damon/ops-common: handle extreme intervals in
>     damon_hot_score()") modified damon_nr_samples_per_aggr() to always
>     return non-zero.  Hence no division-by-zero of the note can happen.

[Severity: Low]
This isn't a bug, but the commit message states it removes an obsolete
division-by-zero comment. However, this comment does not appear to be
removed in the diff below. Was it already removed by an earlier commit
in the series?

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index dea63083917b0..718268df27fa7 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -883,19 +883,18 @@ static unsigned int damon_sample_bp_to_count(
>  	return bp * damon_nr_samples_per_aggr(attrs) / 10000;
>  }

[Severity: High]
This is a pre-existing issue, but can this multiplication silently
overflow on architectures where unsigned int is 32-bit?

If a privileged user configures DAMON attributes via sysfs such that
aggr_interval / sample_interval > 429,496, damon_nr_samples_per_aggr()
will return a large 32-bit unsigned integer. The multiplication
bp * damon_nr_samples_per_aggr(attrs) could then exceed 32 bits, resulting
in incorrect sample counts and potentially flawed memory management policies.

>  
> -/* Convert nr_accesses to access ratio in bp (per 10,000) */
> -static unsigned int damon_nr_accesses_to_accesses_bp(
> -		unsigned int nr_accesses, struct damon_attrs *attrs)
> +/* convert sample count to ratio in bp (per 10,000) */
> +static unsigned int damon_sample_count_to_bp(
> +		unsigned int count, struct damon_attrs *attrs)
>  {
> -	return mult_frac(nr_accesses, 10000, damon_nr_samples_per_aggr(attrs));
> +	return mult_frac(count, 10000, damon_nr_samples_per_aggr(attrs));
>  }

[Severity: High]
This is also a pre-existing issue, but does mult_frac() run into a similar
32-bit overflow?

The mult_frac macro evaluates the remainder multiplication (count * 10000)
as a 32-bit unsigned int. If count >= 429,497, this could overflow, leading
to a flawed ratio calculation.

[ ... ]

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 224a3ecaa0701..9e631b19b184a 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
> @@ -575,14 +575,14 @@ static void damon_test_set_regions(struct kunit *test)
[ ... ]
> -static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test)
> +static void damon_test_sample_count_to_bp(struct kunit *test)
>  {
>  	struct damon_attrs attrs = {
>  		.sample_interval = 10,
>  		.aggr_interval = ((unsigned long)UINT_MAX + 1) * 10
>  	};

[Severity: Medium]
Does this test initialization unconditionally fail on 32-bit architectures?

On 32-bit systems, unsigned long is 32 bits, causing ((unsigned long)UINT_MAX + 1)
to wrap to 0. This makes aggr_interval = 0.

>  
> -	KUNIT_EXPECT_EQ(test, damon_nr_accesses_to_accesses_bp(123, &attrs), 0);
> +	KUNIT_EXPECT_EQ(test, damon_sample_count_to_bp(123, &attrs), 0);
>  }

Because aggr_interval is 0, damon_nr_samples_per_aggr() safely returns 1.
The call to damon_sample_count_to_bp(123, &attrs) then computes
123 * 10000 / 1, which evaluates to 1,230,000. This would fail the
KUNIT_EXPECT_EQ assertion that expects 0.

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