[PATCH] mm/damon: validate addr_unit to be power of 2

[PATCH] mm/damon: validate addr_unit to be power of 2

[Liew Rui Yan]

Problem
=======
The 'addr_unit' must be a power of 2 for correct address alignment
calculations. Previously, writing a non-power-of-2 value
(e.g., addr_unit=3) would be accepted by the sysfs store callback, but
cause kdamond to exit unexpectedly during parameter application, failing
silently without returning an error to userspace.

Solution
========
Add an is_power_of_2() check in damon_commit_ctx() to reject invalid
inputs immediately with -EINVAL. 

When damon_commit_ctx() fails, the kdamond thread terminates as
designed.

The issue is found by sashiko [1].

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

Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
---
 mm/damon/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index db6c67e52d2b..6bad85a47a79 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1330,6 +1330,8 @@ 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 (!src->addr_unit || !is_power_of_2(src->addr_unit))
+		return -EINVAL;
 
 	err = damon_commit_schemes(dst, src);
 	if (err)
-- 
2.53.0

Re: [PATCH] mm/damon: validate addr_unit to be power of 2

[Liew Rui Yan]

Hi SeongJae,

While testing, I noticed that when damon_commit_ctx() returns -EINVAL
due to an invalid 'addr_unit', the kdamond stops running. I recall you
mentioned [1] that the kdamond should only stop for "internal error":

"
I found an issue in the current behavior, and just posted a fix.  Assuming the
fix is merged, the behavior will again be changed.  So, please do this only
after the fix is merged or completely abandoned.

If the fix is merged, the changed behavior will be something like,

"It will continue running with old valid parameters if the parameters are
wrong.  It will stop running if the parameters update was unable to be
completed due to internal error."
"

Is this termination behavior expected for this case?

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

Best regards,
Rui Yan

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

[Liew Rui Yan]

Forwarding Sashiko review for doing discussions via mails.

# review url: https://sashiko.dev/#/patchset/20260327062627.66426-1-aethernet65535@gmail.com
# start of sashiko.dev inline review
commit ceb9522e072d53117ff67157d6f0be7d35919759
Author: Liew Rui Yan <aethernet65535@gmail.com>

mm/damon: validate addr_unit to be power of 2

This commit adds a validation check in damon_commit_ctx() to ensure
addr_unit is a power of 2. This prevents kdamond from exiting
unexpectedly during parameter application when given invalid inputs.

Link: https://lore.kernel.org/20260325025317.86571-1-sj@kernel.org

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index db6c67e52d2b..6bad85a47a79 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -1330,6 +1330,8 @@ 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 (!src->addr_unit || !is_power_of_2(src->addr_unit))

This isn't a bug, but is the !src->addr_unit check needed here?

Looking at is_power_of_2(), it already evaluates to false for an input
of zero, so the compound condition could perhaps be simplified to just
!is_power_of_2(src->addr_unit).

> +		return -EINVAL;
>  
>  	err = damon_commit_schemes(dst, src);
>  	if (err)


# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/20260327062627.66426-1-aethernet65535@gmail.com
#
# hkml [1] generated a draft of this mail.  It can be regenerated
# using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260327062627.66426-1-aethernet65535@gmail.com
#
# [1] https://github.com/sjp38/hackermail

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

[Liew Rui Yan]

> [...]
> > @@ -1330,6 +1330,8 @@ 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 (!src->addr_unit || !is_power_of_2(src->addr_unit))
> 
> This isn't a bug, but is the !src->addr_unit check needed here?
> 
> Looking at is_power_of_2(), it already evaluates to false for an input
> of zero, so the compound condition could perhaps be simplified to just
> !is_power_of_2(src->addr_unit).

You are right. Futhermore, addr_unit_store() also prevents 'addr_unit'
from being 0.

Best regards,
Rui Yan

Re: [PATCH] mm/damon: validate addr_unit to be power of 2

[Liew Rui Yan]

Hi SeongJae,

Regarding the kdamond termination, I have an idea:
Should we avoid terminating kdamond just because of invalid parameters
from user input (e.g., non-power-of-2 'min_region_sz' or 'addr_unit')?

Since a terminated kdamond cannot be easily restarted currently (if it
is not terminated by 'echo N > enabled'), we should probably try to
reduce the probability of such unexpected exits caused by user input.

I'm thinking of validating these parameters early in damon_commit_ctx().
If they are invalid, we could reset 'dst->maybe_corrupted' to 'false'
and return -EINVAL.

This way, since no actual modifications to 'dst' were made before the
check, we can safely clear the 'maybe_corrupted' flag. User will then
receive the error but kdamond should be able to keep running with its
existing configuration instead of exiting.

Does this approach make sense to you?

Best regards,
Rui Yan

Re: [PATCH] mm/damon: validate addr_unit to be power of 2

[SeongJae Park]

Hi Liew,


Unfortunately I'm bit confused about this patch.  Please answer below
questions.

On Fri, 27 Mar 2026 14:26:27 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Problem
> =======
> The 'addr_unit' must be a power of 2 for correct address alignment
> calculations.

It is required to be a power of 2 only if it is smaller than PAGE_SIZE.  Refer
to commit d8fe5d2a3379 ("Docs/mm/damon/design: document the power-of-two
limitation for addr_unit") for more details.

> Previously,

Can we make the timing more explicitly?  I'm confusing when is the time you are
mentioning, since a behavioral change was made recently, by commit c80f46ac228b
("mm/damon/core: disallow non-power of two min_region_sz").

> writing a non-power-of-2 value
> (e.g., addr_unit=3) would be accepted by the sysfs store callback, but
> cause kdamond to exit unexpectedly during parameter application, failing
> silently without returning an error to userspace.

I'm being more confused.  DAMON_SYSFS handles commits synchronously, and return
error to user space when it failed, iirc.  Am I missing something?

> 
> Solution
> ========
> Add an is_power_of_2() check in damon_commit_ctx() to reject invalid
> inputs immediately with -EINVAL. 

If invalid addr_unit already makes damon_commit_ctx() fails and therefore
kdamond exit, why we need more check?

> 
> When damon_commit_ctx() fails, the kdamond thread terminates as
> designed.

Again, I don't get it...


Thanks,
SJ

[...]

Re: [PATCH] mm/damon: validate addr_unit to be power of 2

[Liew Rui Yan]

Hi SeongJae,

Thank you for the detailed explaination and for pointing out the
'PAGE_SIZE' limitation. You are right; and I should have accounted for
that.

However, I performend further testing to address your confusion
regarding kdamond's termination. My patch didn't fix anything.

Here is my reproduction:

# Log (Both with and without this patch):

	# cd /sys/module/damon_lru_sort/parameters/
	# echo Y > enabled 
	# echo 3 > addr_unit 
	# ps aux | rg "[k]damond"
	root    71  0.0 0.0 0   0   ?   I   22:26   0:00 [kdamond.0]
	# echo Y > commit_inputs 
	bash: echo: write error: Invalid argument
	# cat kdamond_pid 
	71
	# ps aux | rg "[k]damond"
	# ... kdamond terminated

I am very sorry for the noise caused by my misunderstanding of the
sashiko review.

The real question I am facing now is:
Should kdamond terminate itself when damon_commit_ctx() fails due to
invalid user inputs?

Best regards,
Rui Yan

Re: [PATCH] mm/damon: validate addr_unit to be power of 2

[SeongJae Park]

On Fri, 27 Mar 2026 22:56:27 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Hi SeongJae,
> 
> Thank you for the detailed explaination and for pointing out the
> 'PAGE_SIZE' limitation. You are right; and I should have accounted for
> that.
> 
> However, I performend further testing to address your confusion
> regarding kdamond's termination. My patch didn't fix anything.
> 
> Here is my reproduction:
> 
> # Log (Both with and without this patch):
> 
> 	# cd /sys/module/damon_lru_sort/parameters/
> 	# echo Y > enabled 
> 	# echo 3 > addr_unit 
> 	# ps aux | rg "[k]damond"
> 	root    71  0.0 0.0 0   0   ?   I   22:26   0:00 [kdamond.0]
> 	# echo Y > commit_inputs 
> 	bash: echo: write error: Invalid argument
> 	# cat kdamond_pid 
> 	71
> 	# ps aux | rg "[k]damond"
> 	# ... kdamond terminated
> 
> I am very sorry for the noise caused by my misunderstanding of the
> sashiko review.
> 
> The real question I am facing now is:
> Should kdamond terminate itself when damon_commit_ctx() fails due to
> invalid user inputs?

It would be better to not terminate.  But not a big deal.  Users could be wary
of the consequence, as long as it is well documented.

From my perspective, therefore, code maintenance is more important.  If we can
make DAMON not terminated by invalid user inputs caused damon_commit_ctx() but
it introduces a significant amount of code complexity, I don't think that's a
good deal.


Thanks,
SJ

[...]

Re: [PATCH] mm/damon: validate addr_unit to be power of 2

[Liew Rui Yan]

Hi SeongJae,

> It would be better to not terminate.  But not a big deal.  Users could be wary
> of the consequence, as long as it is well documented.
> 
> From my perspective, therefore, code maintenance is more important.  If we can
> make DAMON not terminated by invalid user inputs caused damon_commit_ctx() but
> it introduces a significant amount of code complexity, I don't think that's a
> good deal.

Thank you so much for the thoughtful response! I completely agree that
code maintainability should be the priority and we should avoid
unnecessary complexity.

Just to make sure I captured the full context, in my earlier follow-ups
[1][2] (in case they got folded in your email client):

- Does the current kdamond termination on invalid user input align with
  your design goal mentioned in [3] (stopping only for "internal
  errors")?

- Would a lightweight fix be acceptable? For example, performing
  validation at the very beginning of damon_commit_ctx(), and returning
  -EINVAL before setting 'maybe_corrupted' to true. Since no
  modifications to 'dst' would have occurred, kdamond could safely
  continue with its old configuration.

Thank you again for your time, and guidance. :>

[1] https://lore.kernel.org/20260327064517.68131-1-aethernet65535@gmail.com
[2] https://lore.kernel.org/20260327121009.38374-1-aethernet65535@gmail.com
[3] https://lore.kernel.org/20260319151528.86490-1-sj@kernel.org

Best regards,
Rui Yan