[PATCH v4 0/2] mm/damon: validate min_region_size to be power of 2

[PATCH v4 0/2] mm/damon: validate min_region_size to be power of 2

[Liew Rui Yan]

Problem
=======
When a user sets an invalid 'addr_unit' (e.g., 3) via
DAMON_LRU_SORT/DAMON_RECLAIM, 'min_region_sz' becomes a non-power-of-2
value. This value eventually reaches damon_commit_ctx(), which does:

    dst->maybe_corrupted = true;
    if (!is_power_of_2(src->min_region_sz))
        return -EINVAL;

Although -EINVAL is returned, 'maybe_corrupted' is already set. The
running kdamond observers this flag and terminates unexpectedly.

"Unexpected termination" here means the kdamond exits without any user
request (e.g., not by writing 'N' to 'enabled').

User Impact
===========
Once kdamond terminates this way, it cannot be restarted via sysfs
because:

1. DAMON_LRU_SORT/DAMON_RECLAIM is built into the kernel, so it cannot
   be unloaded and reloaded at runtime.
2. Writing 'N' to 'enabled' fails because kdamond no longer exists;
   Writing 'Y' does nothing, as 'enabled' is already Y.

Solution
========
Add an early validation in damon_lru_sort_apply_parameters()
/damon_reclaim_apply_parameters() to check 'min_region_sz' before any
state change occurs. If it is non-power-of-2, return -EINVAL immediately,
preventing 'maybe_corrupted' from being set.

Patch 1 fixes the issue for DAMON_LRU_SORT.
Patch 2 fixes the issue for DAMON_RECLAIM.

Changes from v3
(https://lore.kernel.org/20260403052837.58063-1-aethernet65535@gmail.com)
- Improve commit message: clarify "unexpected termination".
- Add detailed User Impact with reason why kdamond cannot be restarted.
Changes from v2
(https://lore.kernel.org/20260402053756.26606-1-aethernet65535@gmail.com)
- Split the patch into two per-module patches.
- Add Fixes: and Cc: stable tags.
- Elaborate user impact and reproduction steps.
Changes from v1
(https://lore.kernel.org/20260331073231.30060-1-aethernet65535@gmail.com)
- Fix memory leak issue.
Changes from first attempt
(https://lore.kernel.org/20260327062627.66426-1-aethernet65535@gmail.com)
- Renamed the subject.
- Validate min_region_sz rather than addr_unit.

Liew Rui Yan (2):
  mm/damon/lru_sort: validate min_region_size to be power of 2
  mm/damon/reclaim: validate min_region_size to be power of 2

 mm/damon/lru_sort.c | 5 +++++
 mm/damon/reclaim.c  | 5 +++++
 2 files changed, 10 insertions(+)

-- 
2.53.0

[PATCH v4 1/2] mm/damon/lru_sort: validate min_region_size to be power of 2

[Liew Rui Yan]

Problem
=======
When a user sets an invalid 'addr_unit' (e.g., 3) via
DAMON_LRU_SORT, 'min_region_sz' becomes a non-power-of-2
value. This value eventually reaches damon_commit_ctx(), which does:

    dst->maybe_corrupted = true;
    if (!is_power_of_2(src->min_region_sz))
        return -EINVAL;

Although -EINVAL is returned, 'maybe_corrupted' is already set. The
running kdamond observers this flag and terminates unexpectedly.

"Unexpected termination" here means the kdamond exits without any user
request (e.g., not by writing 'N' to 'enabled').

User Impact
===========
Once kdamond terminates this way, it cannot be restarted via sysfs
because:

1. DAMON_LRU_SORT is built into the kernel, so it cannot be unloaded and
   reloaded at runtime.
2. Writing 'N' to 'enabled' fails because kdamond no longer exists;
   Writing 'Y' does nothing, as 'enabled' is already Y.

Reproduction
============
1. Enable DAMON_LRU_SORT
2. Set addr_unit=3
3. Commit inputs via 'commit_inputs'
4. Observe kdamond termination

Solution
========
Add an early validation in damon_lru_sort_apply_parameters() to check
'min_region_sz' before any state change occurs. If it is non-power-of-2,
return -EINVAL immediately, preventing 'maybe_corrupted' from being set.

Fixes: 2e0fe9245d6b ("mm/damon/lru_sort: support addr_unit for DAMON_LRU_SORT")
Cc: <stable@vger.kernel.org> # 6.18.x
Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
---
 mm/damon/lru_sort.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
index 554559d72976..3fd176ef9d9c 100644
--- a/mm/damon/lru_sort.c
+++ b/mm/damon/lru_sort.c
@@ -294,6 +294,11 @@ static int damon_lru_sort_apply_parameters(void)
 	param_ctx->addr_unit = addr_unit;
 	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
 
+	if (!is_power_of_2(param_ctx->min_region_sz)) {
+		err = -EINVAL;
+		goto out;
+	}
+
 	if (!damon_lru_sort_mon_attrs.sample_interval) {
 		err = -EINVAL;
 		goto out;
-- 
2.53.0

[PATCH v4 2/2] mm/damon/reclaim: validate min_region_size to be power of 2

[Liew Rui Yan]

Problem
=======
When a user sets an invalid 'addr_unit' (e.g., 3) via
DAMON_RECLAIM, 'min_region_sz' becomes a non-power-of-2
value. This value eventually reaches damon_commit_ctx(), which does:

    dst->maybe_corrupted = true;
    if (!is_power_of_2(src->min_region_sz))
        return -EINVAL;

Although -EINVAL is returned, 'maybe_corrupted' is already set. The
running kdamond observers this flag and terminates unexpectedly.

"Unexpected termination" here means the kdamond exits without any user
request (e.g., not by writing 'N' to 'enabled').

User Impact
===========
Once kdamond terminates this way, it cannot be restarted via sysfs
because:

1. DAMON_RECLAIM is built into the kernel, so it cannot be unloaded and
   reloaded at runtime.
2. Writing 'N' to 'enabled' fails because kdamond no longer exists;
   Writing 'Y' does nothing, as 'enabled' is already Y.

Reproduction
============
1. Enable DAMON_RECLAIM
2. Set addr_unit=3
3. Commit inputs via 'commit_inputs'
4. Observe kdamond termination

Solution
========
Add an early validation in damon_reclaim_apply_parameters() to check
'min_region_sz' before any state change occurs. If it is non-power-of-2,
return -EINVAL immediately, preventing 'maybe_corrupted' from being set.

Fixes: 7db551fcfb2a ("mm/damon/reclaim: support addr_unit for DAMON_RECLAIM")
Cc: <stable@vger.kernel.org> # 6.18.x
Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
---
 mm/damon/reclaim.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
index 86da14778658..2747eef5919d 100644
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -204,6 +204,11 @@ static int damon_reclaim_apply_parameters(void)
 	param_ctx->addr_unit = addr_unit;
 	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
 
+	if (!is_power_of_2(param_ctx->min_region_sz)) {
+		err = -EINVAL;
+		goto out;
+	}
+
 	if (!damon_reclaim_mon_attrs.aggr_interval) {
 		err = -EINVAL;
 		goto out;
-- 
2.53.0

Re: (sashiko review) [PATCH v4 1/2] mm/damon/lru_sort: validate min_region_size to be power of 2

[Liew Rui Yan]

Sashiko found some issues.

Forwarding full sashiko review in a reply format with my 
inline comments below, for sharing details of my view and
doing followup discussions via mails if needed.

> # review url: https://sashiko.dev/#/patchset/20260410044259.95877-2-aethernet65535@gmail.com
> # start of sashiko.dev inline review
> commit bc048f9325d07815d621e80f3c40408c0210af75
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> 
> mm/damon/lru_sort: validate min_region_size to be power of 2
> 
> This commit adds early validation for min_region_sz in
> damon_lru_sort_apply_parameters(). If the user configures an addr_unit that
> produces a non-power-of-2 min_region_sz, it immediately returns -EINVAL to
> avoid setting maybe_corrupted and unexpectedly terminating kdamond.
> 
> > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > index 554559d729760..3fd176ef9d9c1 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c
> > @@ -294,6 +294,11 @@ static int damon_lru_sort_apply_parameters(void)
> >  	param_ctx->addr_unit = addr_unit;
> >  	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
> >  
> > +	if (!is_power_of_2(param_ctx->min_region_sz)) {
> > +		err = -EINVAL;
> > +		goto out;
> > +	}
> > +
> 
> Is the DAMON sysfs interface also susceptible to this regression?
> 
> When a user configures addr_unit via sysfs and writes 'on' to the state file,
> damon_sysfs_turn_damon_on() builds the context. The min_region_sz is
> calculated directly from addr_unit without a power-of-two check, and then
> damon_start() is called, bypassing the damon_commit_ctx() safeguard.
> 
> Since the DAMON core uses min_region_sz as an alignment mask in macros like
> ALIGN() and ALIGN_DOWN(), can this cause memory corruption or infinite loops
> in functions like damon_fill_regions_holes() if a non-power-of-2 value is
> provided via sysfs?

This issue has been discussed before [1]. While it's a valid concern,
it's outside the scope of this backport patch. SeongJae Park has
indicated he will work on this.

> 
> This patch works around damon_commit_ctx() failing due to !is_power_of_2()
> by performing the check early, but does damon_commit_ctx() still have a
> regression leading to unexpected termination if other allocations fail?

Yes, unexpected termination still exists.

This patch provides a minimal workaround for the specific case of
non-power-of-2 'min_region_sz', which is suitable for backport.

> 
> If damon_commit_ctx() fails midway, for example returning -ENOMEM from
> damon_commit_schemes(), it leaves dst->maybe_corrupted = true and bails
> out without rolling back the partial updates.
> 
> Even the DAMON sysfs interface, which attempts to mitigate this using a
> test_ctx dry-run, could experience a time-of-check to time-of-use race
> where the test allocation succeeds but the final commit allocation fails.
> 
> Since these modules operate under memory pressure, if an -ENOMEM occurs
> during parameter updates, will the running reclaim daemon permanently
> observe maybe_corrupted = true and unexpectedly terminate itself?

You're correct. If damon_commit_ctx() fails midway, it leaves
'maybe_corrupted=true', causing kdamond to terminate permanently. This
is a core error handling issue that needs to be fixed, but it's outside
the scope of this backport patch.

> 
> >  	if (!damon_lru_sort_mon_attrs.sample_interval) {
> >  		err = -EINVAL;
> >  		goto out;
> 
> 
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260410044259.95877-2-aethernet65535@gmail.com

# hkml [1] generated a draft of this mail.  You can regenerate
# this using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260410044259.95877-2-aethernet65535@gmail.com
#
# [1] https://github.com/sjp38/hackermail

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

Best regards,
Rui Yan

Re: (sashiko review) [PATCH v4 2/2] mm/damon/reclaim: validate min_region_size to be power of 2

[Liew Rui Yan]

Sashiko found some issues.

Forwarding sashiko review in a reply format with my inline
comments below, for details of my view and doing discussions
via mails if needed.

> # review url: https://sashiko.dev/#/patchset/20260410044259.95877-3-aethernet65535@gmail.com
> # start of sashiko.dev inline review
> commit 7a816b41f4b321b6a513227bef0e50f3bc1e86f1
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> 
> mm/damon/reclaim: validate min_region_size to be power of 2
> 
> This commit adds an early validation in damon_reclaim_apply_parameters() to
> check if min_region_sz is a power of 2. It attempts to prevent kdamond from
> terminating unexpectedly by catching invalid values before they reach
> damon_commit_ctx().
> 
> > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> > index 86da147786583..2747eef5919d1 100644
> > --- a/mm/damon/reclaim.c
> > +++ b/mm/damon/reclaim.c
> > @@ -204,6 +204,11 @@ static int damon_reclaim_apply_parameters(void)
> >  	param_ctx->addr_unit = addr_unit;
> >  	param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
> 
> Is it possible for a concurrent sysfs write to change addr_unit between these
> two reads, allowing an invalid value to bypass the new validation?
> 
> The global variable addr_unit is read twice without READ_ONCE(), once to set
> param_ctx->addr_unit, and again to calculate param_ctx->min_region_sz.
> 
> If addr_unit is changed from a non-power-of-2 to a power-of-2 between the two
> reads, the validation below might succeed using the new value, while the
> invalid addr_unit is committed to the active context.

I believe this is possible, because the current call to
damon_reclaim_apply_parameters() is asynchronous and does not hold the
'kernel_param_lock' during execution, so this situation should be
possible.

However, I'm still unsure whether your suggested changes should be
included in this patch.

> 
> > +	if (!is_power_of_2(param_ctx->min_region_sz)) {
> > +		err = -EINVAL;
> > +		goto out;
> > +	}
> > +
> 
> Does this early check mask a structural issue in the core API?
> 
> Looking at damon_commit_ctx() in mm/damon/core.c:
> 
> int damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src)
> {
>         ...
>         dst->maybe_corrupted = true;
>         if (!is_power_of_2(src->min_region_sz))
>                 return -EINVAL;
>         ...
> 
> If a different caller passes an invalid min_region_sz, it will still trigger
> this same trap.
> 
> Could the root cause be addressed by moving the maybe_corrupted flag update
> below the validation checks inside damon_commit_ctx()?
> 
> Are there other ways damon_commit_ctx() could leave the context corrupted?
> 
> If a user dynamically enables features that require memory allocation,
> damon_commit_ctx() calls damon_commit_schemes() which attempts to allocate
> memory via damos_commit_quota_goals().
> 
> If this allocation fails with -ENOMEM, it returns an error but leaves
> dst->maybe_corrupted = true on the active context, permanently terminating
> kdamond.
> 

I discussed this in my previous reply. But I realized there might have
been some misundertanding in my previous explanation. What I wanted to
say is that there might indeed be some issues with 'maybe_corrupted' in
damon_commit_ctx() that need adjustment, but that's not within the scope
of this patch.

> Can a concurrent sysfs write cause a divide-by-zero panic in this same
> function?
> 
> Looking further down in damon_reclaim_apply_parameters():
> 
>         if (quota_mem_pressure_us) {
>                 goal = damos_new_quota_goal(DAMOS_QUOTA_SOME_MEM_PSI_US,
>                                 quota_mem_pressure_us);
> 
> The global module parameter quota_mem_pressure_us is read twice without
> READ_ONCE().
> 
> If a concurrent write changes its value to 0 immediately after the if
> condition passes, a quota goal with a target_value of 0 is added.
> 
> When DAMON evaluates the quota score in damos_quota_score(), it uses the
> macro mult_frac(goal->current_value, 10000, goal->target_value). Dividing
> by a goal->target_value of 0 will cause a kernel panic.

I tried to reproduce this issue, but without success. 

However, based on the code, I believe the problem exists because I
didn't find any checks preventing 'goal->target_value=0'. Please let me
know if I've missed anything.

> 
> A similar sequence exists for quota_autotune_feedback.
> 
> >  	if (!damon_reclaim_mon_attrs.aggr_interval) {
> >  		err = -EINVAL;
> >  		goto out;
> >  	}
> 
> 
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260410044259.95877-3-aethernet65535@gmail.com

# hkml [1] generated a draft of this mail.  You can regenerate
# this using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260410044259.95877-3-aethernet65535@gmail.com
#
# [1] https://github.com/sjp38/hackermail

Best regards,
Rui Yan