mm/damon: fix the temporal goal tuner's size quota conversion

damos_goal_tune_esz_bp_temporal() hands the size quota to damos_set_effective_quota() through quota->esz_bp in basis points, and the multiply that gets it there is unchecked. On 32-bit it wraps above 429496 bytes, and to exactly zero for every multiple of 256 MiB. A zero effective quota makes damos_quota_is_full() true on the first test of every charge window, so the scheme applies nothing for as long as the goal is unachieved.

Patch 1 bounds the conversion. Patch 2 pins the boundary in the core kunit suite, where it would fail without patch 1 on any word size.

v2: https://lore.kernel.org/damon/[email protected]/

Changes in v3, all from SJ Park's review; the code is unchanged:
- patch 1: wrap the changelog at 72 columns, say what the impact is and is not, fold the 64-bit boundary into the trigger conditions rather than arguing it separately, and drop the passage on why widening esz_bp is not the fix
- patch 2: wrap the changelog at 72 columns; pick up the Reviewed-by
- drop the documentation patch, which SJ asked to drop

Rebased onto mm-new 87acb5b914c2; v2's base is no longer in that branch.

DAMON kunit on x86_64, on the new base: 36 tests, all passing with both patches; 35 passing and damos_test_esz_goal_temporal failing with patch 2 alone.

Donggeun Yoo (2):
mm/damon/core: prevent size quota overflow in the temporal goal tuner mm/damon/tests/core-kunit: test the temporal tuner's size quota
conversion

 mm/damon/core.c             |  2 +-
 mm/damon/tests/core-kunit.h | 48 +++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

2.53.0

damos_goal_tune_esz_bp_temporal() converts the scheme's size quota into basis points with "quota->esz_bp = quota->sz * 10000", both unsigned long, and damos_set_effective_quota() divides the result back by 10000. quotas/bytes is unbounded; bytes_store() hands it to kstrtoul() as is.

On 32-bit the product wraps for any size quota above ULONG_MAX / 10000, that is 429496 bytes. It lands on exactly zero when the quota is a multiple of 256 MiB, which includes the 1 GiB that Documentation/admin-guide/mm/damon/usage.rst uses in its example, and a zero effective quota makes damos_quota_is_full() true on the first test of every charge window. The scheme then applies nothing while the goal is unachieved. Other wrapped values are simply wrong, and any product below 10000 divides to a zero effective quota too: 429497 gives 0, 500000 gives 70503.

Triggering this needs a scheme with a quota goal, the temporal goal tuner, and a size quota above ULONG_MAX / 10000 -- 429496 bytes on 32-bit, 1844674407370955 on 64-bit -- so it is unlikely to be hit on a tested setup. Nothing is corrupted and nothing leaks. The scheme makes no progress for as long as the goal is unachieved, which is easy to notice, and writing a smaller size quota restores it.

addr_unit does not cover this. It only scales the numbers a paddr context writes to quotas/bytes, so a large enough scaled value wraps just the same, and vaddr and fvaddr contexts take raw byte values.

Bound the multiply. A size quota too large to convert now takes the same ULONG_MAX branch as a scheme with no size quota, so the effective quota becomes ULONG_MAX / 10000 instead of a wrapped value.

---
Measured on i386 under QEMU: one paddr context with a stat scheme, the temporal goal tuner, and one unachieved user_input goal. Each size is written to quotas/bytes and quotas/effective_bytes is read back after update_schemes_effective_quotas.

  quotas/bytes    effective_bytes    effective_bytes
                  before             after
  4096            4096               4096
  429496          429496             429496
  429497          0                  429496
  268435456       0                  429496
  1073741824      0                  429496
  500000          70503              429496
  4294967295      429495             429496
  0               429496             429496

Everything the conversion can hold is unchanged, and 429496 is what the no-size-quota row already produced before the patch.

 mm/damon/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2258b72da7a78..16d4145379a2b 100644
@@ -3274,7 +3274,7 @@ static void damos_goal_tune_esz_bp_temporal(struct damon_ctx *c,
 
 	if (score >= 10000)
 		quota->esz_bp = 0;
-	else if (quota->sz)
+	else if (quota->sz && quota->sz <= ULONG_MAX / 10000)
 		quota->esz_bp = quota->sz * 10000;
 	else
 		quota->esz_bp = ULONG_MAX;

2.53.0

damos_goal_tune_esz_bp_temporal() encodes the size quota in basis points, so the conversion is exact only up to ULONG_MAX / 10000. Pin the three sizes around that boundary: the largest one that fits, the first one that does not, and ULONG_MAX.

---
 mm/damon/tests/core-kunit.h | 48 +++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

index 5ff0436c58441..a0604788bc638 100644
@@ -1929,6 +1929,53 @@ static void damon_test_rand(struct kunit *test)
 	}
 }
 
+static void damos_test_esz_goal_temporal(struct kunit *test)
+{
+	struct damos_access_pattern pattern = {};
+	struct damos_watermarks wmarks = {};
+	struct damos_quota quota = {
+		.goal_tuner = DAMOS_QUOTA_GOAL_TUNER_TEMPORAL,
+	};
+	struct damos_quota_goal *goal;
+	struct damon_ctx *ctx;
+	struct damos *s;
+
+	ctx = damon_new_ctx();
+	KUNIT_ASSERT_NOT_NULL(test, ctx);
+
+	s = damon_new_scheme(&pattern, DAMOS_STAT, 0, &quota, &wmarks,
+			NUMA_NO_NODE);
+	if (!s) {
+		damon_destroy_ctx(ctx);
+		kunit_skip(test, "scheme alloc fail");
+	}
+	damon_add_scheme(ctx, s);
+
+	goal = damos_new_quota_goal(DAMOS_QUOTA_USER_INPUT, 10000);
+	if (!goal) {
+		damon_destroy_ctx(ctx);
+		kunit_skip(test, "quota goal alloc fail");
+	}
+	goal->current_value = 0;
+	damos_add_quota_goal(&s->quota, goal);
+
+	/* The largest size quota the basis-point conversion can hold. */
+	s->quota.sz = ULONG_MAX / 10000;
+	damos_set_effective_quota(ctx, s);
+	KUNIT_EXPECT_EQ(test, s->quota.esz, ULONG_MAX / 10000);
+
+	/* Any larger one saturates instead of wrapping. */
+	s->quota.sz = ULONG_MAX / 10000 + 1;
+	damos_set_effective_quota(ctx, s);
+	KUNIT_EXPECT_EQ(test, s->quota.esz, ULONG_MAX / 10000);
+
+	s->quota.sz = ULONG_MAX;
+	damos_set_effective_quota(ctx, s);
+	KUNIT_EXPECT_EQ(test, s->quota.esz, ULONG_MAX / 10000);
+
+	damon_destroy_ctx(ctx);
+}
+
 static struct kunit_case damon_test_cases[] = {
 	KUNIT_CASE(damon_test_target),
 	KUNIT_CASE(damon_test_regions),

@@ -1965,6 +2012,7 @@ static struct kunit_case damon_test_cases[] = {
 	KUNIT_CASE(damon_test_is_last_region),
 	KUNIT_CASE(damon_test_walk_control_obsolete),
 	KUNIT_CASE(damon_test_rand),
+	KUNIT_CASE(damos_test_esz_goal_temporal),
 	{},
 };
 

2.53.0

As I commented [1] on v2 thread, this version is posted without ensuring the discussion is done, and gave time for others to chime in. I will skip reviewing this version.

[1] https://lore.kernel.org/[email protected]

Thanks,
SJ

[...]