Re: [RFC PATCH 07/11] mm/damon/core: change damon_stop() return type to void

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ABBA deadlock in the error handling path of `damon_start()` when a context fails to start.
--

commit 48348b0ead7556cdf0c68b6bf17ef7a1991b89c8
Author: SJ Park <sj@kernel.org>

mm/damon/core: change damon_stop() return type to void

This commit changes the return type of damon_stop() from int to void,
as it always returned 0 and callers did not check the value.

[ ... ]

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 0845d6b151f81..aa48bf44c2a0b 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1873,14 +1873,13 @@ static void __damon_stop(struct damon_ctx *ctx)
>   * @ctxs:	an array of the pointers for contexts to stop monitoring
>   * @nr_ctxs:	size of @ctxs
>   */
> -int damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
> +void damon_stop(struct damon_ctx **ctxs, int nr_ctxs)
>  {

[Severity: High]
Can this cause an ABBA deadlock in the error handling path of damon_start()
when a context fails to start?

When damon_start() is called with multiple contexts, it acquires damon_lock.
If the first context starts successfully, it spawns a kdamond thread which
begins execution in kdamond_fn().

If the second context fails to start, the error path calls damon_stop() while
still holding damon_lock:

mm/damon/core.c:damon_start() {
    ...
		err = __damon_start(ctxs[i]);
		if (err) {
			damon_stop(ctxs, i);
			break;
		}
    ...
}

damon_stop() eventually calls kthread_stop_put() on the thread for the
first context, which blocks waiting for the thread to exit.

However, the first context's thread attempts to acquire damon_lock in
kdamond_fn() to decrement nr_running_ctxs:

mm/damon/core.c:kdamond_fn() {
    ...
	mutex_lock(&damon_lock);
	nr_running_ctxs--;
    ...
}

Since damon_start() holds damon_lock while waiting for the thread to exit,
and the thread waits for damon_lock to exit, won't this result in a permanent
deadlock?

>  	int i;
>  
>  	for (i = 0; i < nr_ctxs; i++)
>  		/* nr_running_ctxs is decremented in kdamond_fn */
>  		__damon_stop(ctxs[i]);
> -	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704181135.132956-1-sj@kernel.org?part=7