Re: (sashiko review) [PATCH 1/2] mm/damon/core: fix damon_call() vs kdamond_fn() exit race

[SeongJae Park <sj@kernel.org>]

On Fri, 27 Mar 2026 17:43:06 -0700 SeongJae Park <sj@kernel.org> wrote:

> Forwarding Sashiko review for doing discussions via mails.
> 
> # review url: https://sashiko.dev/#/patchset/20260327233319.3528-2-sj@kernel.org
> # start of sashiko.dev inline review
> commit 9cf54c95f0c4bda07c5dbaf4019d0d731f6c58fd
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: fix damon_call() vs kdamond_fn() exit race
> 
> This commit introduces a new call_controls_obsolete flag to prevent
> damon_call() from adding requests to a terminating kdamond context.
> It removes the after-registration DAMON context termination check.
> 
> Link: https://lore.kernel.org/20260325141956.87144-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index db6c67e52d2b..9bcda2765ac9 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> 
> [ ... ]
> 
> > @@ -1629,10 +1604,12 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
> >  	INIT_LIST_HEAD(&control->list);
> >  
> >  	mutex_lock(&ctx->call_controls_lock);
> > +	if (ctx->call_controls_obsolete) {
> > +		mutex_unlock(&ctx->call_controls_lock);
> > +		return -ECANCELED;
> > +	}
> >  	list_add_tail(&control->list, &ctx->call_controls);
> >  	mutex_unlock(&ctx->call_controls_lock);
> > -	if (!damon_is_running(ctx))
> > -		return damon_call_handle_inactive_ctx(ctx, control);
> >  	if (control->repeat)
> >  		return 0;
> >  	wait_for_completion(&control->completion);
> 
> Can this code trigger a use-after-free on control?
> 
> This appears to be a pre-existing issue that remains unfixed. If
> kdamond_fn() is terminating, it executes kdamond_call(ctx, true). This
> function removes the request, sets it as canceled, and calls kfree(control)
> if control->dealloc_on_cancel is true.
> 
> Since ctx->call_controls_lock is dropped right before the control->repeat
> check:
> 
> CPU1 (kdamond_fn)
> kdamond_call(ctx, true)
>     list_del(&control->list);
>     kfree(control);
> 
> CPU2 (damon_call)
>     mutex_unlock(&ctx->call_controls_lock);
>     if (control->repeat)
> 
> Could kdamond_call() free control before damon_call() evaluates
> control->repeat?

No.  kdamond_call(ctx, true) is called only after call_controls_obsolete is
set, under the call_control_lock.  And damon_call() reads the
call_controls_obsolete just after acquiring the call_control_lock, and return
if it is set.

Hence CPU2 in this scenario cannot execute the mutex_unlock() part after CPU1
entered kdamond_call().

> 
> Furthermore, if memory poisoning causes the freed memory to evaluate
> control->repeat as false, would this proceed to call wait_for_completion()
> on the freed memory and cause wait queue corruption?

So this is also wrong.


Thanks,
SJ

[...]