[PATCH 0/2] mm/damon/reclaim: fix addr_unit validation and age truncation

[PATCH 0/2] mm/damon/reclaim: fix addr_unit validation and age truncation

[Josh Law]

Two small fixes for mm/damon/reclaim.c:

Patch 1 rejects non-power-of-2 addr_unit values in the store function.
Without this, DAMON_MIN_REGION_SZ / addr_unit produces a non-power-of-2
min_region_sz that causes undefined behavior in ALIGN() before
damon_commit_ctx() gets a chance to reject it.  Note that
mm/damon/lru_sort.c has the same pattern and likely needs a similar fix.

Patch 2 clamps the min_age / aggr_interval result to UINT_MAX to prevent
silent truncation when assigning to the unsigned int min_age_region field.

Josh Law (2):
  mm/damon/reclaim: reject non-power-of-2 addr_unit
  mm/damon/reclaim: fix min_age_region truncation from unsigned long to
    unsigned int

 mm/damon/reclaim.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/2] mm/damon/reclaim: reject non-power-of-2 addr_unit

[Josh Law]

DAMON_MIN_REGION_SZ / addr_unit is used as min_region_sz, which is
passed to ALIGN() and ALIGN_DOWN() in core.c. These macros require
power-of-2 alignment. When addr_unit is not a power of 2 (e.g., 3),
the division produces a non-power-of-2 min_region_sz, causing silent
undefined behavior in ALIGN before damon_commit_ctx() gets a chance
to reject it.

Validate that addr_unit is a power of 2 in the store function so the
user gets immediate -EINVAL feedback instead of a silent failure.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 mm/damon/reclaim.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
index 43d76f5bed44..9b55df304e51 100644
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -321,7 +321,7 @@ static int damon_reclaim_addr_unit_store(const char *val,
 
 	if (err)
 		return err;
-	if (!input_addr_unit)
+	if (!input_addr_unit || !is_power_of_2(input_addr_unit))
 		return -EINVAL;
 
 	addr_unit = input_addr_unit;
-- 
2.34.1

[PATCH 2/2] mm/damon/reclaim: fix min_age_region truncation from unsigned long to unsigned int

[Josh Law]

min_age / aggr_interval can exceed UINT_MAX with extreme but valid
module parameter values, silently truncating min_age_region and
lowering the cold-page age threshold below what the user intended.
Clamp the result to UINT_MAX to match the field's type.

Signed-off-by: Josh Law <objecting@objecting.org>
---
 mm/damon/reclaim.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
index 9b55df304e51..303c64a56073 100644
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -170,8 +170,8 @@ static struct damos *damon_reclaim_new_scheme(void)
 		.min_nr_accesses = 0,
 		.max_nr_accesses = 0,
 		/* for min_age or more micro-seconds */
-		.min_age_region = min_age /
-			damon_reclaim_mon_attrs.aggr_interval,
+		.min_age_region = min_t(unsigned long, min_age /
+			damon_reclaim_mon_attrs.aggr_interval, UINT_MAX),
 		.max_age_region = UINT_MAX,
 	};
 
-- 
2.34.1

Re: [PATCH 1/2] mm/damon/reclaim: reject non-power-of-2 addr_unit

[SeongJae Park]

On Thu, 19 Mar 2026 16:16:19 +0000 Josh Law <objecting@objecting.org> wrote:

> DAMON_MIN_REGION_SZ / addr_unit is used as min_region_sz, which is
> passed to ALIGN() and ALIGN_DOWN() in core.c. These macros require
> power-of-2 alignment. When addr_unit is not a power of 2 (e.g., 3),
> the division produces a non-power-of-2 min_region_sz, causing silent
> undefined behavior in ALIGN before damon_commit_ctx() gets a chance
> to reject it.

But the non-power-of-2 min_region_sz makes no effect or be used anywhere unless
damon_commit_ctx() is completed without the rejection.  So this is not a real
issue?  Am I missing something?

> 
> Validate that addr_unit is a power of 2 in the store function so the
> user gets immediate -EINVAL feedback instead of a silent failure.
> 
> Signed-off-by: Josh Law <objecting@objecting.org>
> ---
>  mm/damon/reclaim.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

And, I'm adding Sashiko comment with my comments in line.

# review url: https://sashiko.dev/#/patchset/20260319161620.189392-2-objecting@objecting.org

> 
> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> index 43d76f5bed44..9b55df304e51 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c
> @@ -321,7 +321,7 @@ static int damon_reclaim_addr_unit_store(const char *val,
>  
>  	if (err)
>  		return err;
> -	if (!input_addr_unit)
> +	if (!input_addr_unit || !is_power_of_2(input_addr_unit))
>  		return -EINVAL;

: This isn't a bug, but should <linux/log2.h> be explicitly included for the
: is_power_of_2() macro to avoid relying on implicit transitive includes?

Unless it causes a build error, I'd say no.

: 
: Does this same undefined behavior still exist in other DAMON interfaces?
: Looking at damon_lru_sort_addr_unit_store() in mm/damon/lru_sort.c and
: addr_unit_store() in mm/damon/sysfs.c, they appear to still only check
: (!input_addr_unit). Could a user writing a non-power-of-2 value to the
: lru_sort module parameters or sysfs interfaces successfully bypass validation
: and produce a malformed min_region_sz?

Josh already mentioned similar fix may needed on DAMON_LRU_SORT.  I am doubting
if this is a real issue, though, as mentioned above.

>  
>  	addr_unit = input_addr_unit;

: Since addr_unit is a global variable updated here without locks, could there
: be a race condition when it is read during parameter application?
: 
: For instance, in damon_reclaim_apply_parameters() and
: damon_lru_sort_apply_parameters(), addr_unit is read twice sequentially:
: 
:     param_ctx->addr_unit = addr_unit;
:     param_ctx->min_region_sz = max(DAMON_MIN_REGION_SZ / addr_unit, 1);
: 
: If a concurrent write updates addr_unit between these two loads (e.g., from 2
: to 4096), could the context's addr_unit get 2 while min_region_sz gets 1,
: creating a mismatched configuration state? Should READ_ONCE() be used to
: securely cache the global state into a local variable?

Agree.  Nonetheless, orthogonal to this patch.  I will work on this.


Thanks,
SJ

[...]

Re: [PATCH 2/2] mm/damon/reclaim: fix min_age_region truncation from unsigned long to unsigned int

[SeongJae Park]

On Thu, 19 Mar 2026 16:16:20 +0000 Josh Law <objecting@objecting.org> wrote:

> min_age / aggr_interval can exceed UINT_MAX with extreme but valid
> module parameter values, silently truncating min_age_region and
> lowering the cold-page age threshold below what the user intended.
> Clamp the result to UINT_MAX to match the field's type.

I believe the issue is not making real issue in real life.  But fixing this
makes the code cleaner to read, so I think this is better to have.  Thank you!

> 
> Signed-off-by: Josh Law <objecting@objecting.org>

Reviewed-by: SeongJae Park <sj@kernel.org>

> ---
>  mm/damon/reclaim.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> index 9b55df304e51..303c64a56073 100644
> --- a/mm/damon/reclaim.c
> +++ b/mm/damon/reclaim.c
> @@ -170,8 +170,8 @@ static struct damos *damon_reclaim_new_scheme(void)
>  		.min_nr_accesses = 0,
>  		.max_nr_accesses = 0,
>  		/* for min_age or more micro-seconds */
> -		.min_age_region = min_age /
> -			damon_reclaim_mon_attrs.aggr_interval,
> +		.min_age_region = min_t(unsigned long, min_age /
> +			damon_reclaim_mon_attrs.aggr_interval, UINT_MAX),
>  		.max_age_region = UINT_MAX,
>  	};

Sashiko comment.
# review url: https://sashiko.dev/#/patchset/20260319161620.189392-3-objecting@objecting.org

: Could direct access to damon_reclaim_mon_attrs.aggr_interval result in a
: division by zero? If a user writes 0 to the module parameter concurrently via
: sysfs between the validation step and this division, it appears the interval
: could be zero here.

Makes sense, but orthogonal to this patch.  I will work on this.

: 
: Also, since both min_age_region and max_age_region are now clamped or set to
: UINT_MAX, the target matching condition becomes exactly UINT_MAX. In
: mm/damon/core.c (kdamond_merge_regions), a region's age increments on each
: interval via r->age++ without bounds checking.
: 
: Does this mean a region's age will hit UINT_MAX, match for a single interval,
: and then wrap around to 0, effectively defeating the "reclaim if older than
: X" logic?

Good point.  But such case may be too rare to consider...?  Anyway irrelevant
to this patch.  I will take more time to think about this.

: 
: Finally, should similar clamping be applied to other DAMON modules? For
: example, in mm/damon/lru_sort.c, cold_thres and hot_thres are computed from
: unsigned long parameters but typed as unsigned int. Similarly, in
: mm/damon/sysfs-schemes.c, damon_sysfs_mk_scheme assigns unsigned long
: boundaries to the damos_access_pattern fields, which could also result in
: silent truncation.

Yes, similar issue may exist in DAMON_LRU_SORT.  But definitely it is
orthogonal to this patch.  Also, as I mentioned above, I think this change is
good to have but not really fixing real critical issue.  If a nice person send
a patch for this, I'd appreciate.


Thanks,
SJ