Re: (sashiko review) [RFC v5] mm/damon: add synchronous commit for commit_inputs

[SeongJae Park <sj@kernel.org>]

On Fri, 27 Mar 2026 13:50:51 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> Hi SeongJae,
> 
> On Thu, 26 Mar 2026 22:08:22 -0700 SeongJae Park <sj@kernel.org> wrote:
> 
> > [...]
> > > > > I will add a patch in v6 to validate 'addr_unit' in addr_unit_store() to
> > > > > reject non-power-of-2 inputs immediately.
> > > > 
> > > > That's because kdamond_fn() has recently changed to exit the loop if
> > > > damon_commit_ctx() fails in the loop for any reason.  Refer to commit
> > > > 26f775a054c3 ("mm/damon/core: avoid use of half-online-committed context") for
> > > > more details.
> > > > 
> > > > Adding the validation in addr_unit_store() would work, but it could be a kind
> > > > of whack-a-mole game.  I'd prefer doing the validation of all inputs in one
> > > > place, just before the last damon_commit_ctx() call.  DAMON_SYSFS is doing so,
> > > > by making a test context and try committing user input to the test context
> > > > before doing that to the running context.  How about doing so here, too?
> > > 
> > > Thank you for the suggestion. I have now added the check
> > > '!src->addr_unit || ! is_power_of_2(src->addr_unit)' to
> > > damon_commit_ctx().
> > 
> > No, what I'm suggesting is not adding more checks, but reusing the existing
> > checks in damon_commit_ctx().
> 
> I just realized my previous reply might have caused a small
> misunderstanding. My intention was not to _add_ a new check, but to
> _move_ the validation to damon_commit_ctx(). So that patch will only
> modify damon_commit_ctx().

I don't really understand what you mean.  I found your patch [1], and still not
clear.  Let's discuss on the new patch.

> 
> > 
> > And, now I think bit differently.  Given the purpose of this patch is making
> > commit_inputs synchronous, I think making more complete validation check is
> > somewhat that can be done separately.  So my suggestion now is just keeping
> > this part just as is.
> 
> Understood. I will keep this series focused on making 'commit_inputs'
> synchronous, and prepare the validation for 'addr_unit' as a separate
> patch later. Thanks for helping me scope this properly.

Yes, let's discuss on the patch.

[1] https://lore.kernel.org/20260327062627.66426-1-aethernet65535@gmail.com

> 
> > [...]
> > > > So the issue already exists.  But let's ensure the fix is merged before this
> > > > patch, since this patch adds another exploitable path that can consequence in
> > > > whole param_lock deadlock.
> > > 
> > > Should I wait for your fix to be merged into damon/next before I post
> > > the next version?
> > 
> > As long as you agree about the mainline merge order, I think you don't need to
> > wait for my fix.  Please feel free to post next version.
> > 
> > FYI, the latest version of my fix is available [1] on the mailing list.
> > 
> > [1] https://lore.kernel.org/20260327004952.58266-1-sj@kernel.org
> 
> Thank you for your confirmation, and your fix. :>

You're welcome :)


Thanks,
SJ

[...]