[PATCH v2 1/3] mm/damon/core: prevent size quota overflow in the temporal goal tuner

[PATCH v2 1/3] mm/damon/core: prevent size quota overflow in the temporal goal tuner

[Donggeun Yoo]

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.  Documentation/admin-guide/mm/damon/usage.rst
instructs "echo $((1024*1024*1024)) > quotas/bytes", and 1 GiB * 10000 is
2500 * 2^32, so that documented value wraps to exactly zero; 256 MiB and
every multiple of it do the same.  quota->esz then becomes zero while the
goal is not achieved, the trailing "if (quota->sz && quota->sz < esz)" can
only lower esz further, and damos_quota_is_full() is true on the first test
of every charge window, so the scheme applies nothing and the goal is never
approached.  Other sizes are wrong without being zero: 500000 yields 70503.

While the addr_unit parameter effectively mitigates the overflow risk by
scaling down the values written to quotas/bytes, it does not fundamentally
solve the issue.  Theoretically, an overflow can still occur if the scaled
value is exceptionally large.  Furthermore, because addr_unit is exclusive
to the paddr operations set, vaddr and fvaddr contexts remain fully
exposed to this overflow since they take unscaled raw byte values.

The 64-bit boundary is reachable without any scaling: bytes_store() takes
whatever kstrtoul() parses, so a quotas/bytes above 1844674407370955 wraps
the multiply there too.

Bound the conversion, so a size quota it cannot represent falls to the
ULONG_MAX the function already writes for a scheme with no size quota.
Widening esz_bp instead would reach the consist tuner, which runs the same
field through damon_feed_loop_next_input(), unsigned long in and out.  On
32-bit a large size quota then behaves like no size quota rather than like
a dead scheme.

Fixes: af738a6a00c1 ("mm/damon/core: introduce DAMOS_QUOTA_GOAL_TUNER_TEMPORAL")
Cc: <stable@vger.kernel.org> # 7.1.x
Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
---
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, the kdamond is started, 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.

Patch 2 pins the same boundary at ULONG_MAX / 10000 and so runs on any
word size.  Without this patch it fails on x86_64:

  # damos_test_esz_goal_temporal: EXPECTATION FAILED at mm/damon/tests/core-kunit.h:1970
  Expected s->quota.esz == (~0UL) / 10000, but
      s->quota.esz == 0 (0x0)
  # damos_test_esz_goal_temporal: EXPECTATION FAILED at mm/damon/tests/core-kunit.h:1974
  Expected s->quota.esz == (~0UL) / 10000, but
      s->quota.esz == 1844674407370954 (0x68db8bac710ca)

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

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 2258b72da7a78..16d4145379a2b 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -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

Re: [PATCH v2 1/3] mm/damon/core: prevent size quota overflow in the temporal goal tuner

[SJ Park]

On Sun, 20 Sep 2026 11:31:09 +0900 Donggeun Yoo <donggeunyoo.kernel@gmail.com> wrote:

> 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,

Let's break commit message lines with 72 columns limit.

> 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.  Documentation/admin-guide/mm/damon/usage.rst
> instructs "echo $((1024*1024*1024)) > quotas/bytes", and 1 GiB * 10000 is
> 2500 * 2^32, so that documented value wraps to exactly zero; 256 MiB and
> every multiple of it do the same.

Why 256 MiB, not 429,496 bytes?

> quota->esz then becomes zero while the
> goal is not achieved, the trailing "if (quota->sz && quota->sz < esz)" can
> only lower esz further, and damos_quota_is_full() is true on the first test
> of every charge window, so the scheme applies nothing and the goal is never
> approached.

So, the way to work around is updating the size quota to smaller value,
correct?

> Other sizes are wrong without being zero: 500000 yields 70503.
> 
> While the addr_unit parameter effectively mitigates the overflow risk by
> scaling down the values written to quotas/bytes, it does not fundamentally
> solve the issue.  Theoretically, an overflow can still occur if the scaled
> value is exceptionally large.  Furthermore, because addr_unit is exclusive
> to the paddr operations set, vaddr and fvaddr contexts remain fully
> exposed to this overflow since they take unscaled raw byte values.
> 
> The 64-bit boundary is reachable without any scaling: bytes_store() takes
> whatever kstrtoul() parses, so a quotas/bytes above 1844674407370955 wraps
> the multiply there too.

But why a sane user would set such huge number?

Because this patch Cc stable@, let's make super clear about the user impact so
that people don't unnecessarily be scared.  Please mention when the issue can
happen.  It can happen only in certain user setups that probably untested.
Since the consequence is easy to detect (scheme makes no progress always), the
setup is likely untested.  Please also mention it does not cause critical
problems like crashes or leaks, and can be easily worked around by updating the
size quota.

> 
> Bound the conversion, so a size quota it cannot represent falls to the
> ULONG_MAX the function already writes for a scheme with no size quota.

I don't understand the above sentence.  Is the grammar correct?

> Widening esz_bp instead would reach the consist tuner, which runs the same
> field through damon_feed_loop_next_input(), unsigned long in and out.  On
> 32-bit a large size quota then behaves like no size quota rather than like
> a dead scheme.

I don't quite understand above.  could you please elaborate?

> 
> Fixes: af738a6a00c1 ("mm/damon/core: introduce DAMOS_QUOTA_GOAL_TUNER_TEMPORAL")
> Cc: <stable@vger.kernel.org> # 7.1.x
> Signed-off-by: Donggeun Yoo <donggeunyoo.kernel@gmail.com>
> ---
> 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, the kdamond is started, 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.
> 
> Patch 2 pins the same boundary at ULONG_MAX / 10000 and so runs on any
> word size.  Without this patch it fails on x86_64:
> 
>   # damos_test_esz_goal_temporal: EXPECTATION FAILED at mm/damon/tests/core-kunit.h:1970
>   Expected s->quota.esz == (~0UL) / 10000, but
>       s->quota.esz == 0 (0x0)
>   # damos_test_esz_goal_temporal: EXPECTATION FAILED at mm/damon/tests/core-kunit.h:1974
>   Expected s->quota.esz == (~0UL) / 10000, but
>       s->quota.esz == 1844674407370954 (0x68db8bac710ca)
> 
>  mm/damon/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 2258b72da7a78..16d4145379a2b 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -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;

Code change looks good to me.


Thanks,
SJ

[...]

Re: [PATCH v2 1/3] mm/damon/core: prevent size quota overflow in the temporal goal tuner

[Donggeun Yoo]

Thank you for the detailed review.

On Sun, 20 Sep 2026 03:37:13 -0700 SJ Park <sj@kernel.org> wrote:
> Let's break commit message lines with 72 columns limit.

Done in v3, here and in 2/3.

> Why 256 MiB, not 429,496 bytes?

They are two different numbers.  429496 is where the multiply starts
to wrap, and every size above it is wrong.  256 MiB is where the
wrapped value lands on exactly zero: 256 MiB * 10000 is 625 * 2^32, so
on 32-bit the product wraps around 625 times and ends at zero.

> So, the way to work around is updating the size quota to smaller value,
> correct?

Correct.  v3 says so.

> But why a sane user would set such huge number?

They would not, which is why v3 no longer argues the 64-bit case in its
own paragraph.  The threshold is now stated once, as part of what it
takes to reach the bug: above ULONG_MAX / 10000, which is 429496 bytes
on 32-bit and 1844674407370955 on 64-bit.

> Because this patch Cc stable@, let's make super clear about the user
> impact [...]

v3:

  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.

> > Bound the conversion, so a size quota it cannot represent falls to the
> > ULONG_MAX the function already writes for a scheme with no size quota.
>
> I don't understand the above sentence.  Is the grammar correct?

You are right that it is hard to read.  The sentence was too long and
tried to say two things at once.  v3:

  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.

> > Widening esz_bp instead would reach the consist tuner [...]
>
> I don't quite understand above.  could you please elaborate?

Sorry, that paragraph was not clear.  It was meant to explain why I did
not simply widen the type.

The other way to fix the overflow is to make esz_bp wider than unsigned
long, u64 for example.  But esz_bp is not used only here.  The consist
tuner keeps its own value in the same field, and
damos_goal_tune_esz_bp_consist() passes it to
damon_feed_loop_next_input(), which takes unsigned long and returns
unsigned long.  So widening esz_bp means widening that function too,
and the consist tuner would change for a problem it does not have.
Bounding the multiply is one line, and only the temporal tuner is
touched.

I dropped the paragraph in v3.  It argues against a fix nobody
proposed, so it only makes the changelog harder to read.

Thanks,
Donggeun

Re: [PATCH v2 1/3] mm/damon/core: prevent size quota overflow in the temporal goal tuner

[SJ Park]

On Sun, 20 Sep 2026 21:24:30 +0900 Donggeun Yoo <donggeunyoo.kernel@gmail.com> wrote:

> Thank you for the detailed review.
> 
> On Sun, 20 Sep 2026 03:37:13 -0700 SJ Park <sj@kernel.org> wrote:
> > Let's break commit message lines with 72 columns limit.
> 
> Done in v3, here and in 2/3.

Don't post a new version without ensuring the discussion on the current version
is completed.  Also give time for others to chime in.  As a rule of thumbs,
ensure all questions on the thread are answered, and give at least ~1 day since
the last comment on the thread before posting a new version.

I will not review the v3.

> 
> > Why 256 MiB, not 429,496 bytes?
> 
> They are two different numbers.  429496 is where the multiply starts
> to wrap, and every size above it is wrong.  256 MiB is where the
> wrapped value lands on exactly zero: 256 MiB * 10000 is 625 * 2^32, so
> on 32-bit the product wraps around 625 times and ends at zero.

So 256 MiB means nothing special, isn't it?  Why you mention it?

> 
> > So, the way to work around is updating the size quota to smaller value,
> > correct?
> 
> Correct.  v3 says so.
> 
> > But why a sane user would set such huge number?
> 
> They would not, which is why v3 no longer argues the 64-bit case in its
> own paragraph.  The threshold is now stated once, as part of what it
> takes to reach the bug: above ULONG_MAX / 10000, which is 429496 bytes
> on 32-bit and 1844674407370955 on 64-bit.
> 
> > Because this patch Cc stable@, let's make super clear about the user
> > impact [...]
> 
> v3:
> 
>   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.

Let's put "The scheme makes no progress ..." before "so it is unlikely ...".

> 
> > > Bound the conversion, so a size quota it cannot represent falls to the
> > > ULONG_MAX the function already writes for a scheme with no size quota.
> >
> > I don't understand the above sentence.  Is the grammar correct?
> 
> You are right that it is hard to read.  The sentence was too long and
> tried to say two things at once.  v3:
> 
>   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.

Seems unnecessarily verbose.  I'd suggest keeping only the first sentence.

> 
> > > Widening esz_bp instead would reach the consist tuner [...]
> >
> > I don't quite understand above.  could you please elaborate?
> 
> Sorry, that paragraph was not clear.  It was meant to explain why I did
> not simply widen the type.
> 
> The other way to fix the overflow is to make esz_bp wider than unsigned
> long, u64 for example.  But esz_bp is not used only here.  The consist
> tuner keeps its own value in the same field, and
> damos_goal_tune_esz_bp_consist() passes it to
> damon_feed_loop_next_input(), which takes unsigned long and returns
> unsigned long.  So widening esz_bp means widening that function too,
> and the consist tuner would change for a problem it does not have.
> Bounding the multiply is one line, and only the temporal tuner is
> touched.
> 
> I dropped the paragraph in v3.  It argues against a fix nobody
> proposed, so it only makes the changelog harder to read.

Yes, let's drop it.


Thanks,
SJ

[...]

Re: [PATCH v2 1/3] mm/damon/core: prevent size quota overflow in the temporal goal tuner

[Donggeun Yoo]

On Sun, 20 Sep 2026 05:39:58 -0700 SJ Park <sj@kernel.org> wrote:
> Don't post a new version without ensuring the discussion on the current version
> is completed.  Also give time for others to chime in.  As a rule of thumbs,
> ensure all questions on the thread are answered, and give at least ~1 day since
> the last comment on the thread before posting a new version.
>
> I will not review the v3.

I did not know.  I will keep it in mind for the next patches.  Thanks.

> So 256 MiB means nothing special, isn't it?  Why you mention it?

I mentioned it because it is a value users are likely to set, and it wraps
to exactly zero, which I thought could be the worst case.  But the problem
is explained well enough without it, so I am dropping it.

> Let's put "The scheme makes no progress ..." before "so it is unlikely ...".

Will do.

> Seems unnecessarily verbose.  I'd suggest keeping only the first sentence.

Will do.

Thanks,
Donggeun

Re: [PATCH v2 1/3] mm/damon/core: prevent size quota overflow in the temporal goal tuner

[SJ Park]

On Sun, 20 Sep 2026 03:37:13 -0700 SJ Park <sj@kernel.org> wrote:

> On Sun, 20 Sep 2026 11:31:09 +0900 Donggeun Yoo <donggeunyoo.kernel@gmail.com> wrote:
> 
> > 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,
> 
> Let's break commit message lines with 72 columns limit.

Zenghui pointed out [1] the documented recommendation is 75 columns limit.
Let's use 75 columns limit.  Sorry for confusing you.

[1] https://lore.kernel.org/20260920131300.48929-1-sj@kernel.org


Thanks,
SJ

[...]