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

[SeongJae Park <sj@kernel.org>]

On Mon, 30 Mar 2026 14:08:51 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Hi SeongJae,
> 
> On Sun, 29 Mar 2026 08:15:07 -0700 SeongJae Park <sj@kernel.org> wrote:
> 
> > On Sun, 29 Mar 2026 15:51:07 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> > [...]
> > > To confirm, are you suggesting something like the first approach [1]?
> > > 
> > >     if (input_addr_unit < PAGE_SIZE && !is_power_of_2(input_addr_unit))
> > >         return -EINVAL;
> > 
> > Yes.  But the real constraint is min_region_sz, so testing it would be more
> > simple and effective.  E.g.,
> > 
> > 	if (!is_power_of_2(param_ctx->min_region_sz))
> > 		return -EINVAL;
> > > 
> > > [1] https://lore.kernel.org/20260325071709.9699-1-aethernet65535@gmail.com
> 
> Thank you for the example.
> 
> The 'param_ctx' you mentioned confused me for a moment, as it doesn't
> exist in the current addr_unit_store().

I'd suggest to do the validation in commit_inputs handling path
(damon_{lru_sort,reclaim}_apply_parameters()), rather than addr_unit_store().
And param_ctx is in the path.

> But I assume you are suggesting
> that we should validate the result of 'min_region_sz' rather than the
> 'addr_unit' itself, and this check should happen within the
> addr_unit_store() callback.

That's correct.

> 
> So, would something like this in addr_uint_store() be what you expect?
> 
>     if (!input_addr_unit)
>         return -EINVAL;
> 
>     min_region_sz = max(PAGE_SIZE / input_addr_unit, 1);

damon_{lru_sort,reclaim}_apply_parameters() does this already.  And that's why
I suggest to put the validation there.

>     if (!is_power_of_2(min_region_sz))
>         return -EINVAL;

Yes, this kind of validation, but in
damon_{lru_sort,reclaim}_apply_parameters(), please.

Someone might argue it is better to return the error as early as possible (when
writing addr_unit).  But in my humble opinion, the benefit from the user
experience is not that big, while it could make code unnecessarily complicated.

Then someone could ask if we should also move the zero addr_unit input check in
addr_unit_store() into apply_parameters().  Yes, I think we should have done
that in the way, and I made a wrong decision at the time.  But, I think that's
not a big problem, so I would prefer keeping the code and behavior as is,
unless it turns out to be a real problem.

> 
> This keeps the fix local to the modules, and directly enforces the
> power-of-2 constraint on the actual monitoring granularity.

Yes.  But again, I'd prefer doing that in apply_parameters().


Thanks,
SJ

[...]