Re: [RFC PATCH 09/11] mm/damon/core: stop ctx in damon_call() before reruning an error

[SJ Park <sj@kernel.org>]

On Sat, 04 Jul 2026 12:35:01 -0700 SJ Park <sj@kernel.org> wrote:

> On Sat, 04 Jul 2026 18:25:12 +0000 sashiko-bot@kernel.org wrote:
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [High] Concurrent invocations of `__damon_stop()` triggered by canceled `damon_call()`s lead to a double `kthread_stop()` deadlock.
> > --
> > 
> > commit 5b208204f87a1b80335c35ce1b66a42e8f74a1ba
> > Author: SJ Park <sj@kernel.org>
> > 
> > mm/damon/core: stop ctx in damon_call() before reruning an error
> > 
> > This commit updates damon_call() to ensure the DAMON context is explicitly
> > stopped when a call fails or is canceled.
> > 
> > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > index aa48bf44c2a0b..9c15d44084af4 100644
> > > --- a/mm/damon/core.c
> > > +++ b/mm/damon/core.c
> > [ ... ]
> > > @@ -1956,8 +1958,13 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
> > >  		return 0;
> > >  	wait_for_completion(&control->completion);
> > >  	if (control->canceled)
> > > -		return -ECANCELED;
> > > +		goto canceled;
> > >  	return 0;
> > > +
> > > +canceled:
> > > +	__damon_stop(ctx);
> > 
> > [Severity: High]
> > Could this lead to a deadlock if multiple threads invoke damon_call()
> > concurrently on the same context?
> > 
> > If multiple threads are waiting in damon_call() and the kthread exits, it
> > cancels all pending controls and wakes the waiters. These threads will jump
> > to the canceled label and call __damon_stop(ctx) concurrently.
> > 
> > Looking at __damon_stop(), it reads ctx->kdamond without clearing it:
> > 
> > static void __damon_stop(struct damon_ctx *ctx)
> > {
> > 	struct task_struct *tsk;
> > 
> > 	mutex_lock(&ctx->kdamond_lock);
> > 	tsk = ctx->kdamond;
> > 	if (tsk) {
> > 		get_task_struct(tsk);
> > 		mutex_unlock(&ctx->kdamond_lock);
> > 		kthread_stop_put(tsk);
> > 		return;
> > 	}
> > 	mutex_unlock(&ctx->kdamond_lock);
> > }
> > 
> > Does this allow multiple threads to fetch the same task struct and call
> > kthread_stop_put() concurrently? 
> > 
> > Since kthread_stop() waits on the kthread's exit completion which is
> > signaled via complete() (waking only one waiter), would the subsequent
> > threads block forever in kthread_stop(), causing a deadlock?
> 
> Good finding.  But there is no such parallel usage of damon_call() to my
> understanding.
> 
> DAMON_SYSFS calls damon_call() for state file writes.  Multiple threads can
> write the file in parallel.  But the damon_call() from store file write
> callback (state_store()) is protected by global damon_sysfs_lock.
> 
> DAMON modules can call damon_call() for parameters commit and initial start.
> But param file writing is protected by the param_lock.
> 
> This may be better to be documented or make the code more robust.

damon_call() is designed for multiple callers, so this is quite confusing.  I
will make it less confusing like below in the next revision.

"""
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2309,7 +2309,8 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)
        return 0;

 canceled:
-       __damon_stop(ctx);
+       while (damon_is_running(ctx))
+               schedule_timeout_idle(msecs_to_jiffies(100));
        return -ECANCELED;

 }
"""


Thanks,
SJ