Re: [RFC PATCH v2 2/3] mm/damon/lru_sort: detect and use fresh enabled and kdamond_pid values

[SeongJae Park <sj@kernel.org>]

On Sat, 18 Apr 2026 05:30:59 +0000 sashiko-bot@kernel.org wrote:

> Sashiko AI review found 3 potential issue(s):
> - [Critical] Missing `.set` callback in `kdamond_pid_param_ops` will cause a kernel panic if the parameter is passed at boot or via modprobe.

I will fix this in the next revision.

> - [High] Loading the module with `enabled=Y` causes a NULL pointer dereference on `ctx`.
> - [High] Concurrent calls to `damon_lru_sort_turn()` lack locking, leading to list corruption and kernel panics.

Above two are valid points but not caused by this patch.  I will separately
work for those.

> --
> 
> commit 905c8d84b8daa6d4ddb6fc214e81e98101608d53
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/lru_sort: detect and use fresh enabled and kdamond_pid values
> 
> This commit updates DAMON_LRU_SORT to dynamically check the running status
> of kdamond instead of relying on cached parameter values. This ensures the
> status correctly reflects when kdamond stops due to events other than
> explicit user requests.
> 
> > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > index 554559d729760..d284cb336b662 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c
> [ ... ]
> > @@ -386,12 +377,8 @@ static int damon_lru_sort_turn(bool on)
> >  {
> >  	int err;
> >  
> > -	if (!on) {
> > -		err = damon_stop(&ctx, 1);
> > -		if (!err)
> > -			kdamond_pid = -1;
> > -		return err;
> > -	}
> > +	if (!on)
> > +		return damon_stop(&ctx, 1);
> 
> Could concurrent calls to this function lead to list corruption? 
> 
> If a user writes 'Y' to the enabled parameter via sysfs at the same
> time damon_lru_sort_init() calls damon_lru_sort_turn(true) during module
> initialization, damon_commit_ctx() might concurrently modify ctx->schemes
> and ctx->targets without internal locking.

Hmm, good finding.  But the race existis even before this patch.  I will
separately work on this.

> 
> [ ... ]
> > @@ -430,10 +414,16 @@ module_param_cb(addr_unit, &addr_unit_param_ops, &addr_unit, 0600);
> >  MODULE_PARM_DESC(addr_unit,
> >  	"Scale factor for DAMON_LRU_SORT to ops address conversion (default: 1)");
> >  
> > +static bool damon_lru_sort_enabled(void)
> > +{
> > +	if (!ctx)
> > +		return false;
> > +	return damon_is_running(ctx);
> > +}
> > +
> >  static int damon_lru_sort_enabled_store(const char *val,
> >  		const struct kernel_param *kp)
> >  {
> > -	bool is_enabled = enabled;
> >  	bool enable;
> >  	int err;
> 
> Is it possible to hit a NULL pointer dereference on ctx when loading the
> module with enabled=Y? 
> 
> If the DAMON core is already initialized (e.g. built-in), damon_initialized()
> would return true, bypassing the early exit in this function. This could
> lead to calling damon_lru_sort_turn(true) before damon_lru_sort_init() has
> run, causing damon_commit_ctx() to operate on a NULL ctx pointer.

Hmm, good finding.  This is a race of a type that is same to the above one.
And again the race exists even before this patch.  I will separately work on
this.

> 
> [ ... ]
> > +static const struct kernel_param_ops kdamond_pid_param_ops = {
> > +	.get = damon_lru_sort_kdamond_pid_load,
> > +};
> > +
> > +/*
> > + * PID of the DAMON thread
> > + *
> > + * If DAMON_LRU_SORT is enabled, this becomes the PID of the worker thread.
> > + * Else, -1.
> > + */
> > +module_param_cb(kdamond_pid, &kdamond_pid_param_ops, NULL, 0400);
> 
> Does this cause a NULL pointer dereference if the parameter is passed at
> boot or via modprobe?
> 
> The kernel parameter parsing code in parse_one() doesn't check if .set is
> NULL before invoking it:
> 
>     err = params[i].ops->set(val, &params[i]);
> 
> A dummy .set function returning an error might be needed to prevent a NULL
> pointer dereference when parsing command line arguments.

Good point.  I will fix this in the next revision like below.

'''
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -412,6 +412,16 @@ module_param_cb(enabled, &enabled_param_ops, &enabled, 0600);
 MODULE_PARM_DESC(enabled,
        "Enable or disable DAMON_RECLAIM (default: disabled)");

+static int damon_reclaim_kdamond_pid_store(const char *val,
+               const struct kernel_param *kp)
+{
+       /*
+        * kdamond_pid is read-only, but kernel command line could write it.
+        * Do nothing here.
+        */
+       return 0;
+}
+
 static int damon_reclaim_kdamond_pid_load(char *buffer,
                const struct kernel_param *kp)
 {
@@ -426,6 +436,7 @@ static int damon_reclaim_kdamond_pid_load(char *buffer,
 }

 static const struct kernel_param_ops kdamond_pid_param_ops = {
+       .set = damon_reclaim_kdamond_pid_store,
        .get = damon_reclaim_kdamond_pid_load,
 };
'''


Thanks,
SJ

[...]