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

[SeongJae Park <sj@kernel.org>]

On Sun, 29 Mar 2026 01:44:09 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> On Sat, 28 Mar 2026 07:13:23 -0700 SeongJae Park <sj@kernel.org> wrote:
> 
> > > On Sat, 28 Mar 2026 10:26:51 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> > >
> > > > [...]
> > > > - 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.
> > > 
> > > I suggested you to try something similar to what DAMON_SYSFS is doing.  But I
> > > didn't get your response to the idea yet.  Could you please let me know what do
> > > you think about the approach?
> > 
> > I was thinking this one more time.  So you want to ensure DAMON_RECLAIM and
> > DAMON_LRU_SORT not passing wrong addr_unit to damon_commit_ctx(), right?  And
> > that's required because exisitng inputs validations of DAMON_RECLAIM and
> > DAMON_LRU_SORT are not checking that.  Why don't you add the just one more
> > check there?
> 
> Apologize for the confusion in my previous explanation. To clarify, my
> proposal was indeed focused on implementing a lightweight fix directly
> within damon_commit_ctx(), rather than adding separate checks in
> DAMON_RECLAIM and DAMON_LRU_SORT.
> 
> The goal is to safely handle the context state transition. Specifically,
> I want to validate inputs before marking the context as potentially
> corrupted. Here are two approaches to achieve this:
> 
> Option 1 - Using a label for cleanup:
> 
>     damon_commit_ctx(...)
>     {
>         err = 0;
> 
>         dst->maybe_corrupted = true;
>         if (...) {
>             err = -EINVAL;
>             goto out_reset;
>         }
> 
>         ... other code ...
> 
>     out_reset:
>         dst->maybe_corrupted = false;
>         return err;
>     }
> 
> Option 2 - Deferring the 'maybe_corrupted' assignment:
> 
>     damon_commit_ctx(...)
>     {
>         err = 0;
> 
>         if (...)
>             return -EINVAL;
> 
>         /*
>          * Only mark as pontentially corrupted once
>          * we start modifying dst.
>          */
>         dst->maybe_corrupted = true;
> 
>         ... other code ...
>     }
> 
> I think Option 2 is cleaner as it avoids unnecessary state flipping.

Liew, I suggeted two options before.  But you are skipping providing your
opinion to those, and adding yet more options.  That makes me difficult to
follow the conversation.  Could you please answer to my suggestions and make a
consensus about those, first?


Thanks,
SJ

[...]