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

Liew Rui Yan
2026-03-26T06:15:58Z

Hi SeongJae,

On Wed, 25 Mar 2026 07:19:56 -0700, SeongJae Park <sj@kernel.org> wrote:

> Hi Liew,
>
> On Wed, 25 Mar 2026 15:17:09 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
>
> > Hi SeongJae,
> >
> > > Forwarding Sashiko review for doing discussions via mails.
> > >
> > > # review url: https://sashiko.dev/#/patchset/20260325013939.18167-1-aethernet65535@gmail.com
> > > # start of sashiko.dev inline review
> > > commit 60ccea4154b0c58741fae2323454a5a9496b67fa
> > > Author: Liew Rui Yan <aethernet65535@gmail.com>
> > >
> > > mm/damon: add synchronous commit for commit_inputs
> > >
> > > Writing invalid parameters to sysfs followed by commit_inputs=Y previously
> > > failed silently. This patch uses damon_call() to synchronously commit
> > > parameters in the kdamond thread's safe context, returning validation errors
> > > immediately to userspace.
> > >
> > > Link: https://lore.kernel.org/20260318153731.97470-1-aethernet65535@gmail.com
> > >
> > > > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > > > index 554559d72976..a2410f648b51 100644
> > > > --- a/mm/damon/lru_sort.c
> > > > +++ b/mm/damon/lru_sort.c
> >
> > Thank you for forwarding the review from Sashiko.dev!
>
> Thank you for reviewing the review.
>
> >
> > > > +static int damon_lru_sort_commit_inputs_store(const char *val,
> > > > + const struct kernel_param *kp)
> > > > +{
> > > > + bool commit_inputs_request;
> > > > int err;
> > > > + struct damon_call_control control = {
> > > > + .fn = damon_lru_sort_commit_inputs_fn,
> > > > + .data = ctx,
> > > > + .repeat = false,
> > > > + };
> > > >
> > > > - if (!commit_inputs)
> > > > + err = kstrtobool(val, &commit_inputs_request);
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > + if (!commit_inputs_request)
> > > > return 0;
> > > >
> > > > - err = damon_lru_sort_apply_parameters();
> > > > - commit_inputs = false;
> > > > - return err;
> > > > + /*
> > > > + * Skip damon_call() during early boot or when kdamond is
> > > > + * not running to avoid NULL pointer dereference.
> > > > + */
> > > > + if (!ctx)
> > > > + return -EBUSY;
> > > > +
> > > > + err = damon_call(ctx, &control);
> > >
> > > Can this sequence lead to a system-wide deadlock on kernel_param_lock?
> > >
> > > Because damon_lru_sort_commit_inputs_store() is a sysfs .set callback, it
> > > executes with the global kernel_param_lock held.
> > >
> > > If kdamond_fn terminates (e.g., due to an invalid parameter like addr_unit=3
> > > causing a non-power-of-2 min_region_sz), it processes its exit sequence by
> > > calling kdamond_call(ctx, true) to flush pending requests. After flushing, it
> > > eventually sets ctx->kdamond = NULL at the very end of the thread.
> > >
> > > If a user writes to commit_inputs exactly in the window after
> > > kdamond_call() finishes but before ctx->kdamond = NULL is set:
> > >
> > > Thread 1 (kdamond_fn):
> > > kdamond_call(ctx, true);
> > >
> > > Thread 2 (sysfs write):
> > > damon_call(ctx, &control);
> > > if (damon_is_running(ctx))
> > > wait_for_completion(&control.completion);
> > >
> > > Thread 1 (kdamond_fn):
> > > ctx->kdamond = NULL;
> > >
> > > Thread 2 will sleep indefinitely in wait_for_completion(). Because Thread 2
> > > holds the global kernel_param_lock, this permanently freezes all module
> > > parameter sysfs interfaces system-wide.
> >
> > I have verified this behavior with the following test case:
> >
> > # cd /sys/module/damon_lru_sort/parameters/
> > # echo Y > enabled
> > # ps aux | grep "[k]damond"
> > root 70 0.0 0.0 0 0 ? I 12:16 0:00 [kdamond.0]
>
> FYI, you can read kdamond_pid parameter.
>
> > # echo 3 > addr_unit
> > # echo Y > commit_inputs
> > bash: echo: write error: Invalid argument
> > # ps aux | grep "[k]damond"
> >
> > ... kdamond has exited unexpectedly
> >
> > 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().

> >
> > > > + if (err) {
> > > > + /*
> > > > + * If kdamond is not running, damon_call() returns -EINVAL.
> > > > + * Remap it to -EBUSY to better reflect that the operation
> > > > + * failed due to the current state of the system.
> > > > + */
> > > > + if (err == -EINVAL && !damon_is_running(ctx))
> > > > + return -EBUSY;
> > > > +
> > > > + return err;
> > > > + }
> > > > +
> > > > + return control.return_code;
> > > > }
> > >
> > > > diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
> > > > index 86da14778658..4cbd67e08d56 100644
> > > > --- a/mm/damon/reclaim.c
> > > > +++ b/mm/damon/reclaim.c
> > >
> > > [ ... ]
> > >
> > > > + /*
> > > > + * Skip damon_call() during early boot or when kdamond is
> > > > + * not running to avoid NULL pointer dereference.
> > > > + */
> > > > + if (!ctx)
> > > > + return -EBUSY;
> > > > +
> > > > + err = damon_call(ctx, &control);
> > >
> > > Does this code share the same kernel_param_lock deadlock potential as
> > > damon_lru_sort_commit_inputs_store() described above?
> >
> > As we discussed in the RFC-v4 thread [1], this is a false positive.
> > Since 'enabled=N' and 'commit_inputs=Y' are both serialized by the
> > global 'kernel_param_lock', and kthreads cannot be forcibly terminated
> > by userspace signals, the completion signal in damon_call() will always
> > be reached in a controlled manner.
>
> TL; DR: I think the deadlock can happen. But that's a separate issue.
>
> Let's suppose the user writes 'Y' to commit_inputs, and damon_commit_ctx()
> fails due to an allocation failure. The write to commit_inputs will complete,
> and kdmond_fn() will start its exit sequence.
>
> The user can write 'Y' to commit_inputs again, when kdamond_fn() finished the
> last kdamond_call() with 'cancel=True', but before it unset the ->kdamond
> field.
>
> Then, the deadlock that sashiko pointed out could happen.
>
> The deadlock is due to the incomplete design of damon_call() cleanup. And I
> think the same deadlock is already available with DAMON_SYSFS. I will work on
> fixing this.

I really appreciate your dedication to DAMON.

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

Best regards,
Rui Yan