Re: [PATCH] mm/damon/core: avoid use of half-online-committed context

[SeongJae Park]

On Thu, 19 Mar 2026 19:48:49 -0700 Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Thu, 19 Mar 2026 07:52:17 -0700 SeongJae Park <sj@xxxxxxxxxx> wrote:
> 
> > One major usage of damon_call() is online DAMON parameters update.  It
> > is done by calling damon_commit_ctx() inside the damon_call() callback
> > function.  damon_commit_ctx() can fail for two reasons: 1) invalid
> > parameters and 2) internal memory allocation failures.  In case of
> > failures, the damon_ctx that attempted to be updated (commit
> > destination) can be partially updated (or, corrupted from a
> > perspective), and therefore shouldn't be used anymore.  The function
> > only ensures the damon_ctx object can safely deallocated using
> > damon_destroy_ctx().
> > 
> > The API callers are, however, calling damon_commit_ctx() only after
> > asserting the parameters are valid, to avoid damon_commit_ctx() fails
> > due to invalid input parameters.  But it can still theoretically fail if
> > the internal memory allocation fails.  In the case, DAMON may run with
> > the partially updated damon_ctx.  This can result in unexpected
> > behaviors including even NULL pointer dereference in case of
> > damos_commit_dests() failure [1].  Such allocation failure is arguably
> > too small to fail, so the real world impact would be rare.  But, given
> > the bad consequence, this needs to be fixed.
> > 
> > Avoid such partially-committed (maybe-corrupted) damon_ctx use by saving
> > the damon_commit_ctx() failure on the damon_ctx object.  For this,
> > introduce damon_ctx->maybe_corrupted field.  damon_commit_ctx() sets it
> > when it is failed.  kdamond_call() checks if the field is set after each
> > damon_call_control->fn() is executed.  If it is set, ignore remaining
> > callback requests and return.  All kdamond_call() callers including
> > kdamond_fn() also check the maybe_corrupted field right after
> > kdamond_call() invocations.  If the field is set, break the
> > kdamond_fn() main loop so that DAMON sill doesn't use the context that
> > might be corrupted.
> 
> I guess you saw the AI review?
> 	https://sashiko.dev/#/patchset/20260319145218.86197-1-sj%40kernel.org

Yes :)  Pasting it and my reply in line below.  TL; DR: We need a fixup that
attached below.

: > --- a/mm/damon/core.c
: > +++ b/mm/damon/core.c
: > @@ -1252,6 +1252,7 @@ int damon_commit_ctx(struct damon_ctx *dst, struct damon_ctx *src)
: >  {
: >       int err;
: >
: > +     dst->maybe_corrupted = true;
: >       if (!is_power_of_2(src->min_region_sz))
: >               return -EINVAL;
: 
: Could this prematurely mark the context as corrupted?
: 
: If the power-of-2 validation fails, the function returns -EINVAL without
: modifying dst at all. However, because maybe_corrupted is already set to
: true, wouldn't the kdamond thread assume its state is corrupted and
: unconditionally shut down?
: 
: Should the flag be set immediately prior to the first mutating operation
: like damon_commit_schemes() instead?

Good suggestion.  But marking it as corrupted here doesn't make something
really bad.  Meanwhile, ksensKeep being aware of what function here is really
making change to the struct will make it complicated to maintain in long term.
I'd prefer keeping the code as is for the simplicity.

: 
: [ ... ]
: 
: > @@ -2678,6 +2680,8 @@ static void kdamond_call(struct damon_ctx *ctx, bool cancel)
: >                       complete(&control->completion);
: >               else if (control->canceled && control->dealloc_on_cancel)
: >                       kfree(control);
: > +             if (ctx->maybe_corrupted)
: > +                     break;
: >       }
: 
: Does this early break cause a deadlock during cancellation?
: 
: When damon_commit_ctx() fails, it sets ctx->maybe_corrupted = true.
: This corruption triggers an exit from the main loop in kdamond_fn() to the
: done: label, which invokes kdamond_call(ctx, true) to cancel and complete
: all remaining pending callbacks.
: 
: However, inside kdamond_call(), this loop now unconditionally checks
: if (ctx->maybe_corrupted) break; at the end of each iteration. Because
: the flag is true, won't the loop process exactly one callback and then
: break, leaving the rest of the callbacks in ctx->call_controls unprocessed?
: 
: If the worker thread then exits, wouldn't any callers waiting on
: wait_for_completion(&control->completion) for those remaining callbacks
: hang indefinitely?
: 
: Would it make sense to bypass this break condition when cancel is true?

Good catch!  Andrew, could you please add below attaching fixup?


Thanks,
SJ

[...]
=== >8 ===