mm/damon: validate min_region_size to be power of 2

# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit bc048f9325d07815d621e80f3c40408c0210af75
Author: Liew Rui Yan <[email protected]>

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.

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

Quoted 13 lines

mm/damon/lru_sort.c +5 -0

index 554559d729760..3fd176ef9d9c1 100644
@@ -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 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?

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?

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

Quoted 4 lines

mm/damon/lru_sort.c +0 -0

 	if (!damon_lru_sort_mon_attrs.sample_interval) {
 		err = -EINVAL;
 		goto out;

# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/[email protected]

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.

Quoted 33 lines

# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit bc048f9325d07815d621e80f3c40408c0210af75
Author: Liew Rui Yan <[email protected]>

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.

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

Quoted 13 lines

mm/damon/lru_sort.c +5 -0

index 554559d729760..3fd176ef9d9c1 100644
@@ -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.

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 10 lines

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.

Quoted 3 lines

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.

Quoted 9 lines

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

Quoted 5 lines

mm/damon/lru_sort.c +0 -0

 	if (!damon_lru_sort_mon_attrs.sample_interval) {
 		err = -EINVAL;
 		goto out;

# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/[email protected]

Quoted 1 line · On Fri, 10 Apr 2026 17:40:04 +0800 Liew Rui Yan <[email protected]>…

Sashiko found some issues.

Clarifying what is the following action for the sashiko finding would be helpful. For example, if you say "Sashiko found some issues, so I will post a new versiuon soon", and if I'm busy, I can save my time by saying "Ok, then I will skip or delaying review of this version and wait for your next review".

[...]

Agreed. This was unclear to me in previous disucssions, though. I still agree it is out of the scope of this patch. But now I think we need to let users force-restart. Adding this to my todo list.

Quoted 8 lines · On Fri, 10 Apr 2026 06:55:00 -0700 SeongJae Park <[email protected]> wrote:

Quoted 1 line · On Fri, 10 Apr 2026 17:40:04 +0800 Liew Rui Yan <[email protected]>…

Sashiko found some issues.

Clarifying what is the following action for the sashiko finding would be helpful. For example, if you say "Sashiko found some issues, so I will post a new versiuon soon", and if I'm busy, I can save my time by saying "Ok, then I will skip or delaying review of this version and wait for your next review".

Okay, I will keep that in mind next time I foward a Sashiko's Review. Thank you for you suggestion.

Quoted 5 lines

[...]

Agreed. This was unclear to me in previous disucssions, though. I still agree it is out of the scope of this patch. But now I think we need to let users force-restart. Adding this to my todo list.

Just to make sure - is this the same issue that my recent RFC patch [1] aims to address? I want to make sure we're not duplicating efforts.

I'm still actively working on that patch, and I plan to send the next version next week. I've been holding off because I didn't want to send multiple patches in parallel.

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

1. Let's hold this patch series. Andrew, please don't merge this for now until this discussion is completed.

Quoted 4 lines · On Sat, 11 Apr 2026 00:46:10 +0800 Liew Rui Yan <[email protected]>…

Quoted 2 lines · On Fri, 10 Apr 2026 06:55:00 -0700 SeongJae Park <[email protected]> wrote:

On Fri, 10 Apr 2026 17:40:04 +0800 Liew Rui Yan <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

[...]

Agreed. This was unclear to me in previous disucssions, though. I still agree it is out of the scope of this patch. But now I think we need to let users force-restart. Adding this to my todo list.

Just to make sure - is this the same issue that my recent RFC patch [1] aims to address? I want to make sure we're not duplicating efforts.

Hmm, this makes me confused about how we ended up working on this series, then.

Quoted 4 lines

I'm still actively working on that patch, and I plan to send the next version next week. I've been holding off because I didn't want to send multiple patches in parallel.

Ok, seems I dropped a ball. I was working like AI bot that only works with limited and nearly fresh context for each mail that on my inbox. I will work on making another thing for tracking this kind of parallel works with good context. But since I already dropped the ball for this, I'd like to make sure we are on the same page. So I'd suggest below.

1. Let's hold this patch series. Andrew, please don't merge this for now until this discussion is completed.
2. Please summarize your parallel works in progress with the context about how you decided to do that in the way, with summaries of our previous discussions.

Could you please do those, Liew?

Quoted 28 lines · On Fri, 10 Apr 2026 10:00:50 -0700 SeongJae Park <[email protected]> wrote:

Quoted 4 lines · On Sat, 11 Apr 2026 00:46:10 +0800 Liew Rui Yan <[email protected]>…

Quoted 2 lines · On Fri, 10 Apr 2026 06:55:00 -0700 SeongJae Park <[email protected]> wrote:

On Fri, 10 Apr 2026 17:40:04 +0800 Liew Rui Yan <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

[...]

Agreed. This was unclear to me in previous disucssions, though. I still agree it is out of the scope of this patch. But now I think we need to let users force-restart. Adding this to my todo list.

Just to make sure - is this the same issue that my recent RFC patch [1] aims to address? I want to make sure we're not duplicating efforts.

Hmm, this makes me confused about how we ended up working on this series, then.

Quoted 4 lines

I'm still actively working on that patch, and I plan to send the next version next week. I've been holding off because I didn't want to send multiple patches in parallel.

Ok, seems I dropped a ball. I was working like AI bot that only works with limited and nearly fresh context for each mail that on my inbox. I will work on making another thing for tracking this kind of parallel works with good context. But since I already dropped the ball for this, I'd like to make sure we are on the same page. So I'd suggest below.

Quoted 7 lines

1. Let's hold this patch series. Andrew, please don't merge this for now until this discussion is completed.
2. Please summarize your parallel works in progress with the context about how you decided to do that in the way, with summaries of our previous discussions.

Could you please do those, Liew?

TL;DR - There is no dependency between my parallel (2) works. I never intended to merge these two works into a single series because they address the problem at different level.

Yes, here is the summary of my parallel works and how I decided to structure them.

My current parallel works
=========================
Series A - min_region_sz power-of-2 validation (this patch)
- Status: You gave Reviewed-by, asked Andrew to merge.
- Scope: Small, standalone fix for specific issue.
- Dependency: None

Series B - reset parameters (enabled/kdamond_pid) on unexpected termination (RFC [1])
- Status: Preparing V2.
- Scope: Reset 'enabled' and 'kdamond_pid' when kdamond terminated unexpectedly.
- Dependency: None

Summary of Series B
===================
The design evolved through our discussion:
1. Extended 'struct damon_ctx' with 'thread_status' pointers
- SJ pointed out: "This feels too much extension of core API for a problem that can more simply be fixed."

2. Alternative proposals discussed:
- Option 1: Termination callback in core API (Too heavy for backport)
- Option 2: Override '.get' operator for parameters (code duplication)
- Option 3: On-demamd correction in enabled_store() (passive)

3. Final direction comfirmed:
- SJ suggested: "Can't we catch damon_commit_ctx() failure from the calling place?"
- We agreed: Simple, backportable fix is the priority.
- Decision: Reset 'enabled' and 'kdamond_pid' immediately when damon_commit_ctx() fails, add a fallback in the damon_turn() 'N' path.

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

Thank you for sharing these. But seems even this context is incomplete from my perspective...

The Real Context
----------------

The real start of the context is probably the question [1] that you asked on 2026-03-18. At that time, we found committing wrong parameters stops DAMON without an user-visible error. We agreed DAMON being killed is no problem but the absence of user-visible error is a minor user experience problem that better to be improved. So you planned to make the user experience improvement.

Apparently series A is the followup of that. The first version was posted on 2026-03-26, according to the changelog of this patch series. I can show I was saying the patch was confusing on the thread.

While the review of the confusing series A was ongoing, you posted [3] the RFC of series B on 2026-03-31. This patch reported one important finding: the silent DAMON stop is not just a minor user experience issue but a serious bug because it cannot be started again.

And this made things much more confusing. There are multiple ways to trigger the issue. Wrong addr_unit commit is just one of the ways. We discussed the way to fix the real issue. So, by looking back this, I think you should prioritized series B from the point, or make suer series A is only for the minor user experience improvement. Or asked me what to prioritize. You didn't and I missed the fact that you are also working on series A.

But you just continued posting new versions of series A. I was wrongly thinking that is still the minor user experience improvement. I still think the patches were implicitly saying so. I'm not sure if it was intentional or not. But definitely it was confusing me. On 2026-04-02, I started feeling I'm missing some of the contexts, and asked you more clarification of user impacts
[4] and full history [5]. You posted v3 [6] right after I asked the question, even without answering the question. Only from this point it became clear sereis A is not just a minor user experience improvement but a critical bug fix. Now I think you should clarify this can also fix one trigger point of the critical bug but the real fix is work in progress, and this is till only a minor user experience improvement. But because you didn't, and my poor memory is volatile, at this point I was thinking this all the work you are doing for the series B-exposed bug.

In the response to the sashiko review on this thread, therefore, I was thinking you are thinking the wrong addr_unit commit is the only way to trigger the bug. I didn't want to ask you to work from the beginning to fix the entire bug, so I was saying fixing the real bug for all exploit points is out of the scope of this bug.

But now it is turned out that you were aware of the other ways to trigger the bug, and didn't transparentl and explicitly exposing that.

So I withdraw what I told on the reply to the sashiko review. And appreciate sashiko developers again for giving us a chance to finding this.

Next Steps
----------

So, what to do? Please prioritize series B, if you still willing to do. It is ok to keep doing series A, but only as the minor user experience improvement. Clearly explain the whole context you are aware of. Don't Cc stable@ for series A, as it is only an incomplete fix of it. The fix of the one trigger point is just a side effect.

And For Future Contributions
----------------------------

Liew, I really appreciate your contributions. You found and shared important DAMON bugs. But apparently your communication has many rooms to improve, at least for poort DAMON maintainer who have to work with only limited resources including the poort and volatile memory. I find I was asking clarifications to your mails multiple times. I have to say it was even frustrating sometimes and definitely took quite amount of my resource. Meanwhile, from my uncautiously biased perspective, you were only adding more traffic and confusions.

This makes me disappointed and even suspect your intention... I really hate myself suspecting someone. But we are in the world of bad actors that now gained the power of AI. We actually had a suspicious case from DAMON contributors recently, do you remember?

I want to still believe you, so I will do so. Please feel free to keep contributing to DAMON as long as that's what you want to do. But, please aware of the fact that DAMON maintainer has poor volatile memory and working with limited resource, and try to make every conversation super clear and transparent, from next time.

[1] https://lore.kernel.org/[email protected]
[2] https://lore.kernel.org/[email protected]
[3] https://lore.kernel.org/[email protected]
[4] https://lore.kernel.org/[email protected]
[5] https://lore.kernel.org/[email protected]
[6] https://lore.kernel.org/[email protected]

Hi SeongJae,

Thank you for your detailed explanation and for being patient with me. I sincerely apologize for the confusion and the extra workload my unclear communication has caused you. It was never my intention to hide context or game the system; I simply misjudged the priorities and failed to provide the full picture.

I want to continue contributing to DAMON. I truly enjoy working on it, and I'm grateful for the opportunity.

Quoted 8 lines

Next Steps
----------

So, what to do? Please prioritize series B, if you still willing to do. It is ok to keep doing series A, but only as the minor user experience improvement. Clearly explain the whole context you are aware of. Don't Cc stable@ for series A, as it is only an incomplete fix of it. The fix of the one trigger point is just a side effect.

Yes, I absolutely want to continue contributing to DAMON.

I realize most of this confusion came from me misinterpreting your words or making assumptions without asking. So this time, I want to be explicit:
1. I will remove the Cc stable@

2. Since you suggested prioritizing Series B, would you prefer me to send a revised v5 of Series A now, or should I wait until Series B is settled to avoid more noise in your inbox?

3. I will focus on Series B starting now.

Hi Liew,

On Mon, 13 Apr 2026 03:04:55 +0800 Liew Rui Yan <[email protected]> wrote:

[...]

Quoted 9 lines

Quoted 8 lines

Next Steps
----------

So, what to do? Please prioritize series B, if you still willing to do. It is ok to keep doing series A, but only as the minor user experience improvement. Clearly explain the whole context you are aware of. Don't Cc stable@ for series A, as it is only an incomplete fix of it. The fix of the one trigger point is just a side effect.

Quoted 6 lines

Yes, I absolutely want to continue contributing to DAMON.

I realize most of this confusion came from me misinterpreting your words or making assumptions without asking. So this time, I want to be explicit:
1. I will remove the Cc stable@

Please also make the description of the patch clear.

I'd like to avoid delaying series B too long, though. The bug is not very critical, but better to be fixed sooner than later. If you have no enough capacity, please feel free to hand it over to me.

[...]

Quoted 14 lines · On Sun, 12 Apr 2026 14:37:19 -0700 SeongJae Park <[email protected]> wrote:

Hi Liew,

On Mon, 13 Apr 2026 03:04:55 +0800 Liew Rui Yan <[email protected]> wrote:

[...]

Quoted 9 lines

Quoted 8 lines

Next Steps
----------

So, what to do? Please prioritize series B, if you still willing to do. It is ok to keep doing series A, but only as the minor user experience improvement. Clearly explain the whole context you are aware of. Don't Cc stable@ for series A, as it is only an incomplete fix of it. The fix of the one trigger point is just a side effect.

Quoted 8 lines

Quoted 6 lines

Yes, I absolutely want to continue contributing to DAMON.

I realize most of this confusion came from me misinterpreting your words or making assumptions without asking. So this time, I want to be explicit:
1. I will remove the Cc stable@

Please also make the description of the patch clear.

I've been thinking about what you meant by "make the desciption of the patch clear" for Series A.

Here's my current understanding:

"
This patch only fixes the case where kdamond terminates because 'min_region_sz' is not power-of-2. Other causes of unexpected termination (e.g., memory allocation failures) are not fixed by this patch. [Seires B] will address the general problem.
"

Is this what you had in mind? Or is there something else you think I should add or change?

Quoted 4 lines

I'd like to avoid delaying series B too long, though. The bug is not very critical, but better to be fixed sooner than later. If you have no enough capacity, please feel free to hand it over to me.

Yes, I have enough capacity. My exams are not until a couple of weeks later, so I can focus on Series B now.

If my schedule gets too busy later, I will let you know and hand it over as you suggested. Thank you for offering that.

Quoted 1 line · On Tue, 14 Apr 2026 02:48:45 +0800 Liew Rui Yan <[email protected]>…

[...]

To my understanding, you will make series B merged before this one, right? If that's the case, you may not need to add this description at all. Rather, you may better to clarify this is only a minor user experience improvement.

[...]

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 <[email protected]> # 6.18.x

Signed-off-by Liew Rui Yan <[email protected]>

# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit 7a816b41f4b321b6a513227bef0e50f3bc1e86f1
Author: Liew Rui Yan <[email protected]>

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().

mm/damon/reclaim.c static int damon_reclaim_apply_parameters(void) · line 205

Quoted 7 lines

mm/damon/reclaim.c +0 -0

index 86da147786583..2747eef5919d1 100644
@@ -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.

mm/damon/reclaim.c static int damon_reclaim_apply_parameters(void) · line 211

Quoted 6 lines

mm/damon/reclaim.c +5 -0

+	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.

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.

A similar sequence exists for quota_autotune_feedback.

mm/damon/reclaim.c static int damon_reclaim_apply_parameters(void)

Quoted 4 lines

mm/damon/reclaim.c +0 -0

 	if (!damon_reclaim_mon_attrs.aggr_interval) {
 		err = -EINVAL;
 		goto out;
 	}

# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/[email protected]

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.

Quoted 23 lines

# review url: https://sashiko.dev/#/patchset/[email protected]
# start of sashiko.dev inline review
commit 7a816b41f4b321b6a513227bef0e50f3bc1e86f1
Author: Liew Rui Yan <[email protected]>

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().

mm/damon/reclaim.c static int damon_reclaim_apply_parameters(void) · line 205

Quoted 7 lines

mm/damon/reclaim.c +0 -0

index 86da147786583..2747eef5919d1 100644
@@ -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?

Quoted 6 lines

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.

Quoted 28 lines

mm/damon/reclaim.c static int damon_reclaim_apply_parameters(void) · line 211

Quoted 7 lines

mm/damon/reclaim.c +5 -0

+	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?

Quoted 8 lines

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.

Quoted 13 lines

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().

Quoted 6 lines

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.

Quoted 4 lines

A similar sequence exists for quota_autotune_feedback.

Quoted 8 lines

mm/damon/reclaim.c static int damon_reclaim_apply_parameters(void)

Quoted 4 lines

mm/damon/reclaim.c +0 -0

 	if (!damon_reclaim_mon_attrs.aggr_interval) {
 		err = -EINVAL;
 		goto out;
 	}

# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/[email protected]

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

Best regards,
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 <[email protected]> # 6.18.x

Signed-off-by Liew Rui Yan <[email protected]>

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/[email protected])
- Improve commit message: clarify "unexpected termination".
- Add detailed User Impact with reason why kdamond cannot be restarted.