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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxx

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

[...]