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

[Liew Rui Yan <aethernet65535@gmail.com>]

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.

> 
> I remember I was saying this kind of change would be a kind of whack-a-mole and
> therefore prefer DAMON_SYSFS type damon_commit_ctx() based checking.  But
> that's not a small change.  Just adding yet another simple check on existing
> validation logic should be fine and small.
>
> [...]

Best regards,
Rui Yan