mm/damon: update probe hits for runtime parameter commits

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

Pre-existing issues:
- [High] Multiplication overflow in DAMON access rate calculations due to 32-bit arithmetic. --

--- Patch [1]: [RFC PATCH 1/6] mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/ --- commit 928ef68b58766df0da13e507ad449f7ba9d5c79d
Author: SJ Park <[email protected]>

mm/damon/core: s/damon_max_nr_accesses()/damon_nr_samples_per_aggr()/

This commit renames damon_max_nr_accesses() to damon_nr_samples_per_aggr() to better describe that it returns the number of samples DAMON checks for each region per aggregation interval.

mm/damon/core.c static unsigned int damon_age_for_new_attrs(unsigned int age, · line 883

Quoted 10 lines

mm/damon/core.c +1 -1

index 390e00b3685ef..e710e2789085e 100644
@@ -880,7 +880,7 @@ static unsigned int damon_age_for_new_attrs(unsigned int age,
 static unsigned int damon_accesses_bp_to_nr_accesses(
 		unsigned int accesses_bp, struct damon_attrs *attrs)
 {
-	return accesses_bp * damon_max_nr_accesses(attrs) / 10000;
+	return accesses_bp * damon_nr_samples_per_aggr(attrs) / 10000;

[Severity: High]
This isn't a bug introduced by this patch, but can this multiplication in damon_accesses_bp_to_nr_accesses() overflow 32-bit arithmetic?

If the sample interval is configured to a small value and the aggregation interval to a large value, damon_nr_samples_per_aggr() can return a large number. Multiplying accesses_bp by this large number wraps around modulo 2^32 since both operands are 32-bit unsigned integers, resulting in an incorrectly reduced access count.

mm/damon/core.c static unsigned int damon_accesses_bp_to_nr_accesses( · line 896

Quoted 10 lines

mm/damon/core.c +1 -1

 }
 
 /*

@@ -893,7 +893,7 @@ static unsigned int damon_accesses_bp_to_nr_accesses(
 static unsigned int damon_nr_accesses_to_accesses_bp(
 		unsigned int nr_accesses, struct damon_attrs *attrs)
 {
-	return mult_frac(nr_accesses, 10000, damon_max_nr_accesses(attrs));
+	return mult_frac(nr_accesses, 10000, damon_nr_samples_per_aggr(attrs));

[Severity: High]
This is a pre-existing issue, but does the use of mult_frac() in damon_nr_accesses_to_accesses_bp() also suffer from a 32-bit overflow?

When nr_accesses is sufficiently large, the internal calculation in mult_frac() overflows 32-bit arithmetic.

Quoted 3 lines

mm/damon/core.c +0 -0

 }
 

[ ... ]

mm/damon/lru_sort.c static int damon_lru_sort_apply_parameters(void) · line 307

Quoted 11 lines

mm/damon/lru_sort.c +1 -1

index e8c389ad3226f..7e077084cb03a 100644
@@ -303,7 +303,7 @@ static int damon_lru_sort_apply_parameters(void)
 		goto out;
 
 	err = -ENOMEM;
-	hot_thres = damon_max_nr_accesses(&attrs) *
+	hot_thres = damon_nr_samples_per_aggr(&attrs) *
 		hot_thres_access_freq / 1000;

[Severity: High]
This isn't a bug introduced by this patch, but does this calculation in damon_lru_sort_apply_parameters() overflow on 32-bit architectures?

Multiplying the sample count by hot_thres_access_freq overflows 32-bit arithmetic on 32-bit systems when the sample count is large.

Quoted 3 lines

mm/damon/lru_sort.c +0 -0

 	hot_scheme = damon_lru_sort_new_hot_scheme(hot_thres);
 	if (!hot_scheme)

[ ... ]

mm/damon/ops-common.c int damon_hot_score(struct damon_ctx *c, struct damon_region *r, · line 115

Quoted 10 lines

mm/damon/ops-common.c +1 -1

index 6bdd1cfd3863a..89c694f6e4fa8 100644
@@ -112,7 +112,7 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
 	int hotness;
 
 	freq_subscore = r->nr_accesses * DAMON_MAX_SUBSCORE /
-		damon_max_nr_accesses(&c->attrs);
+		damon_nr_samples_per_aggr(&c->attrs);

[Severity: High]
This is a pre-existing issue, but could this calculation in damon_hot_score() result in a 32-bit overflow?

Multiplying r->nr_accesses by DAMON_MAX_SUBSCORE (which is 100) overflows 32-bit arithmetic if the number of samples is near UINT_MAX. This might cause hot memory to be incorrectly identified as cold.

Quoted 4 lines

mm/damon/ops-common.c +0 -0

 	age_in_sec = (unsigned long)r->age * c->attrs.aggr_interval / 1000000;
 	if (age_in_sec)

Signature · 1 line

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

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

Pre-existing issues:
- [High] 32-bit integer overflow in conversions between basis points (bp) and sample counts due to large number of samples per aggregation interval. --

--- Patch [2]: [RFC PATCH 2/6] mm/damon/core: s/accesses_bp_to_nr_accesses/sample_bp_to_count/ --- commit 26f76c0115cf94c7177c6588b38ff64dabf63ab2
Author: SJ Park <[email protected]>

mm/damon/core: s/accesses_bp_to_nr_accesses/sample_bp_to_count/

This commit renames accesses_bp_to_nr_accesses() to damon_sample_bp_to_count() to better reflect that it converts a samples ratio (in basis points) to a count. It also updates the callsites and comments to match the new name.

Quoted 7 lines

mm/damon/core.c +0 -0

index e710e2789085e..ca530faaa4f83 100644
@@ -876,11 +876,11 @@ static unsigned int damon_age_for_new_attrs(unsigned int age,
 	return age * old_attrs->aggr_interval / new_attrs->aggr_interval;
 }

mm/damon/core.c static unsigned int damon_age_for_new_attrs(unsigned int age, · line 883

Quoted 10 lines

mm/damon/core.c +4 -4

 
-/* convert access ratio in bp (per 10,000) to nr_accesses */
-static unsigned int damon_accesses_bp_to_nr_accesses(
-		unsigned int accesses_bp, struct damon_attrs *attrs)
+/* convert sample ratio in bp (per 10,000) to count */
+static unsigned int damon_sample_bp_to_count(
+		unsigned int bp, struct damon_attrs *attrs)
 {
-	return accesses_bp * damon_nr_samples_per_aggr(attrs) / 10000;
+	return bp * damon_nr_samples_per_aggr(attrs) / 10000;

[Severity: High]
This isn't a bug introduced by this patch, but can the product of bp and damon_nr_samples_per_aggr() overflow 32 bits?

Since both bp and the return value of damon_nr_samples_per_aggr() are 32-bit unsigned integers, their product can exceed 32 bits if the number of samples per aggregation interval is large (greater than ~429,496).

For example, a 1ms sample interval and a 10-minute aggregation interval would result in 600,000 samples. If bp is 10,000 (100%), the multiplication 10,000 * 600,000 overflows 32 bits to 1,705,032,704, resulting in a truncated count of 170,503 instead of 600,000.

A similar overflow can occur in damon_nr_accesses_to_accesses_bp() when evaluating (nr_accesses % nr_samples) * 10000 inside mult_frac() for large nr_accesses values, falsely marking hot memory as cold.

Quoted 2 lines

mm/damon/core.c +0 -0

 }

Signature · 1 line

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Obsolete divide-by-zero comment and skip logic were not removed in the KUnit test, contradicting the commit message and core code. --

--- Patch [3]: [RFC PATCH 3/6] mm/damon/core: s/nr_accesses_to_accesses_bp/sample_count_to_bp/ --- commit aaafc980a7ea0e3c63783a3dbd0d43aab65ff523
Author: SJ Park <[email protected]>

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 describe its functionality. It also mentions removing obsolete comments regarding a division-by-zero bug that was previously fixed.

Quoted 4 lines

mm/damon/tests/core-kunit.h +0 -0

index 0124f83b39b83..86eae3a590354 100644

[ ... ]

mm/damon/tests/core-kunit.h static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test) · line 595

Quoted 12 lines

mm/damon/tests/core-kunit.h +1 -1

@@ -585,7 +585,7 @@ static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test)
 	/*
 	 * In some cases such as 32bit architectures where UINT_MAX is
 	 * ULONG_MAX, attrs.aggr_interval becomes zero.  Calling
-	 * damon_nr_accesses_to_accesses_bp() in the case will cause
+	 * damon_sample_count_to_bp() in the case will cause
 	 * divide-by-zero.  Such case is prohibited in normal execution since
 	 * the caution is documented on the comment for the function, and
 	 * damon_update_monitoring_results() does the check.  Skip the test in

@@ -594,7 +594,7 @@ static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test)
 	if (!attrs.aggr_interval)
 		kunit_skip(test, "aggr_interval is zero.");

[Severity: Medium]
The commit message mentions that the obsolete comment about division-by-zero is removed because damon_nr_samples_per_aggr() now always returns non-zero.

Should this comment and the associated kunit_skip logic also be removed from damon_test_sample_count_to_bp()?

It appears to still reference the caution comment that was removed from the core code, and it continues to guard against a divide-by-zero that can no longer happen.

Signature · 2 lines

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

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

Pre-existing issues:
- [High] 32-bit integer overflow in damon_sample_bp_to_count() and damon_sample_count_to_bp() corrupts memory access tracking. --

--- Patch [4]: [RFC PATCH 4/6] mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/ --- commit cb8a54b34f772f70862ab31ccf30eea4a5cb2263
Author: SJ Park <[email protected]>

mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/

This commit renames damon_nr_accesses_for_new_attrs() to damon_nr_samples_for_new_attrs() to clarify its general use for any positive sample count.

mm/damon/core.c static unsigned int damon_sample_count_to_bp( · line 892

Quoted 8 lines

mm/damon/core.c +0 -0

index 718268df27fa7..cda318806b65b 100644
@@ -890,7 +890,7 @@ static unsigned int damon_sample_count_to_bp(
 	return mult_frac(count, 10000, damon_nr_samples_per_aggr(attrs));
 }
 

[Severity: High]
This is a pre-existing issue, but can this math silently overflow when the sample count is large?

In mm/damon/core.c:damon_sample_count_to_bp(), the mult_frac() macro inherits the 32-bit precision of 'count'. If the user configures sysfs such that max samples exceeds ~429,496, the intermediate remainder multiplication can wrap around UINT_MAX.

For instance, 450,000 accesses out of 500,000 maximum samples will silently overflow during mult_frac() calculation, resulting in a basis point of 410 (4.1% hot) instead of 9000 (90% hot).

Similarly, in mm/damon/core.c:damon_sample_bp_to_count():

    static unsigned int damon_sample_bp_to_count(
            unsigned int bp, struct damon_attrs *attrs)
    {
        return bp * damon_nr_samples_per_aggr(attrs) / 10000;
    }

The multiplication occurs in 32-bit precision before the division by 10000. If damon_nr_samples_per_aggr() exceeds ~429,496, this will also overflow.

This drastic corruption could cause DAMOS to incorrectly evaluate region hotness, potentially swapping out highly accessed memory. Would it make sense to cast these to a 64-bit type before multiplication to avoid the overflow?

Quoted 6 lines

mm/damon/core.c +1 -1

-static unsigned int damon_nr_accesses_for_new_attrs(unsigned int nr_accesses,
+static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr_accesses,
 		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
 {
 	return damon_sample_bp_to_count(

[ ... ]

Signature · 1 line

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

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

Pre-existing issues:
- [High] 32-bit arithmetic overflow in interval scaling functions corrupts core metrics. --

--- Patch [5]: [RFC PATCH 5/6] mm/damon/core: update probe hits for new parameter commit --- commit 7e26d9c01ec3d36d75d519b11f48ef36f78ec868
Author: SJ Park <[email protected]>

mm/damon/core: update probe hits for new parameter commit

Users can update DAMON parameters at runtime. If the samples and/or aggregation intervals are updated, monitoring results depending on the intervals should also be updated. This commit ensures probe_hits are properly updated to reflect new intervals, similar to age and nr_accesses.

Quoted 4 lines

mm/damon/core.c +0 -0

index cda318806b65b..9a3ff0fe55a61 100644

[ ... ]

Quoted 4 lines

mm/damon/core.c +0 -0

@@ -898,10 +898,32 @@ static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr_accesses,
 			new_attrs);
 }
 

mm/damon/core.c static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr_accesses, · line 910

Quoted 10 lines

mm/damon/core.c +10 -0

+static void damon_update_probe_hits(struct damon_region *r,
+		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs,
+		bool aggregating, struct damon_ctx *ctx)
+{
+	struct damon_probe *p;
+	int i = 0;
+
+	damon_for_each_probe(p, ctx) {
+		r->last_probe_hits[i] = damon_nr_samples_for_new_attrs(
+				r->last_probe_hits[i], old_attrs, new_attrs);

[Severity: High]
This is a pre-existing issue, but can the scaling logic called here overflow and corrupt the core metrics?

Looking at the underlying functions called by
damon_nr_samples_for_new_attrs():

mm/damon/core.c:damon_sample_bp_to_count() {
    return bp * damon_nr_samples_per_aggr(attrs) / 10000;
}

and

mm/damon/core.c:damon_sample_count_to_bp() {
    return mult_frac(count, 10000, damon_nr_samples_per_aggr(attrs));
}

If a user configures a large aggregation to sampling interval ratio (e.g., 1 second aggregation and 1 microsecond sampling), the ratio can exceed 429K.

When damon_sample_count_to_bp() scales a count of 500,000, the internal multiplication in mult_frac() (count * 10000) evaluates to 5,000,000,000, which overflows the 32-bit unsigned integer maximum and yields an incorrect value.

Similarly, damon_sample_bp_to_count() performs 32-bit multiplication which can overflow if bp * samples exceeds the 32-bit limit.

Should these scaling calculations cast to 64-bit to prevent silent wrap-around and wildly inaccurate statistics?

Quoted 10 lines

mm/damon/core.c +9 -0

+		if (!aggregating)
+			r->probe_hits[i] = damon_nr_samples_for_new_attrs(
+					r->probe_hits[i], old_attrs,
+					new_attrs);
+		else
+			r->probe_hits[i] = 0;
+		i++;
+	}
+}

Signature · 1 line

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