Re: (sashiko review) [RFC PATCH] mm/damon: reset thread status parameters upon kdamond termination

[Liew Rui Yan <aethernet65535@gmail.com>]

Forwarding Sashiko review for doing discussions via mails.

# review url: https://sashiko.dev/#/patchset/20260330164347.12772-1-aethernet65535@gmail.com
# start of sashiko.dev inline review
commit 636b179cc5df5a638d7988bd9de56ce4d8602f4a
Author: Liew Rui Yan <aethernet65535@gmail.com>

mm/damon: reset thread status parameters upon kdamond termination

This commit introduces a thread status structure in the DAMON context to
link the kdamond state with module parameters. It updates these parameters
when the kdamond thread finishes to reflect its actual state.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index db6c67e52d2b..6c71203beec5 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -2941,6 +2944,14 @@ static void kdamond_init_ctx(struct damon_ctx *ctx)
>  	}
>  }
>  
> +static void damon_update_thread_status(struct damon_ctx *ctx)
> +{
> +	if (ctx->thread_status.kdamond_pid)
> +		*ctx->thread_status.kdamond_pid = -1;
> +	if (ctx->thread_status.enabled)
> +		*ctx->thread_status.enabled = false;

Can this write race with the user enabling the module? If a user writes 'Y'
to 'enabled', the store function spawns kdamond and prepares to set 'enabled'
to true. If kdamond exits immediately (e.g., due to invalid targets), could
this line asynchronously set 'enabled' to false before the store function
overwrites it with true?

If so, the system would be left in a state where the thread is dead but
'enabled' is true. Subsequent attempts to write 'N' to 'enabled' would fail
on damon_stop(), leaving the module permanently locked.

> +}
> +
>  /*
>   * The monitoring daemon that runs as a kernel thread
>   */

[ ... ]

> @@ -3065,17 +3076,23 @@ static int kdamond_fn(void *data)
>  	kdamond_call(ctx, true);
>  	damos_walk_cancel(ctx);
>  
> -	pr_debug("kdamond (%d) finishes\n", current->pid);
>  	mutex_lock(&ctx->kdamond_lock);
>  	ctx->kdamond = NULL;
>  	mutex_unlock(&ctx->kdamond_lock);
>  
> +	if (ctx->thread_status.enabled && *ctx->thread_status.enabled)

Can this access freed memory? If the kdamond_lock is dropped and
ctx->kdamond is NULL, damon_is_running(ctx) becomes false. If a concurrent
sysfs operation removes the context, could ctx be freed by damon_destroy_ctx()
before these lines execute, causing a use-after-free and memory corruption in
damon_update_thread_status(ctx)?

> +		pr_debug("kdamond (%d) crashed\n", current->pid);

Does this log normal user-requested shutdowns as crashes? When a user stops
the module by writing 'N' to 'enabled', the parameter store blocks on
damon_stop(). Since the global 'enabled' variable is still true at this
point, won't this incorrectly print a crash message instead of finishing
normally?

> +	else
> +		pr_debug("kdamond (%d) finishes\n", current->pid);
> +
>  	mutex_lock(&damon_lock);
>  	nr_running_ctxs--;
>  	if (!nr_running_ctxs && running_exclusive_ctxs)
>  		running_exclusive_ctxs = false;
>  	mutex_unlock(&damon_lock);
>  
> +	damon_update_thread_status(ctx);
> +
>  	return 0;
>  }

# end of sashiko.dev inline review
# review url: https://sashiko.dev/#/patchset/20260330164347.12772-1-aethernet65535@gmail.com
#
# hkml [1] generated a draft of this mail.  It can be regenerated
# using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260330164347.12772-1-aethernet65535@gmail.com
#
# [1] https://github.com/sjp38/hackermail