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

On Sat, 18 Apr 2026 08:36:55 -0700 SeongJae Park <[email protected]> wrote:

> On Sat, 18 Apr 2026 05:30:59 +0000 [email protected] 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 <[email protected]>
> > 
> > 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.

Seems I was hallucinated.  DAMON_LRU_SORT is not loadable, so this race cannot
happen.  I will ignore this report.

> 
> > 
> > [ ... ]
> > > @@ -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.

I will ignore the above report for the same reason.


Thanks,
SJ

[...]