Re: [RFC PATCH v1.2 1/2] mm/damon/stat: add a parameter for reading kdamond pid

[SeongJae Park <sj@kernel.org>]

On Wed, 15 Apr 2026 21:06:01 -0700 SeongJae Park <sj@kernel.org> wrote:

> On Wed, 15 Apr 2026 20:35:45 -0700 SeongJae Park <sj@kernel.org> wrote:
> 
> > TL; DR: Sashiko found good point.  I will fix it in the next revision.
> > 
> > On Thu, 16 Apr 2026 00:59:35 +0000 sashiko-bot@kernel.org wrote:
> > 
> > > Sashiko AI review found 1 potential issue(s):
> > > - [Medium] Stale `kdamond_pid` exposed to userspace if `damon_call()` fails during startup.
> > > --
> > > 
> > > commit 46eb0a4278ccd2dec190db28b78b0766b8865901
> > > Author: SeongJae Park <sj@kernel.org>
> > > 
> > > mm/damon/stat: add a parameter for reading kdamond pid
> > > 
> > > This commit exposes the pid of the DAMON_STAT kdamond via a new read-only
> > > module parameter named kdamond_pid, standardizing usage with other DAMON
> > > modules.
> > > 
> > > > diff --git a/mm/damon/stat.c b/mm/damon/stat.c
> > > > index 99ba346f9e325..4901e9a7c3398 100644
> > > > --- a/mm/damon/stat.c
> > > > +++ b/mm/damon/stat.c
> > > 
> > > [ ... ]
> > > 
> > > > @@ -260,6 +269,14 @@ static int damon_stat_start(void)
> > > >  		damon_stat_context = NULL;
> > > >  		return err;
> > > >  	}
> > > > +	kdamond_pid = damon_kdamond_pid(damon_stat_context);
> > > > +	if (kdamond_pid < 0) {
> > > > +		err = kdamond_pid;
> > > > +		kdamond_pid = -1;
> > > > +		damon_destroy_ctx(damon_stat_context);
> > > > +		damon_stat_context = NULL;
> > > > +		return err;
> > > > +	}
> > > >  
> > > >  	damon_stat_last_refresh_jiffies = jiffies;
> > > >  	call_control.data = damon_stat_context;
> > > 
> > > If damon_call() fails right after this and returns an error,
> > > damon_stat_start() will pass that error back to its caller and the module
> > > will revert to a disabled state.
> > > 
> > > Does this leave a stale kdamond_pid exposed to userspace? The newly added
> > > documentation states: "If DAMON_STAT is enabled, this becomes the PID of
> > > the worker thread. Else, -1." Since the initialization fails, the module
> > > remains disabled, but kdamond_pid isn't reset to -1 on this error path.
> > 
> > Nice catch.  I will do the kdamond_pid setup after damon_call() success, like
> > below.
> 
> Wait...  The same question was also raised [1] by Sashiko on the previous
> version.  And I was wrongly saying it is not this patch's fault.  That was
> completely wrong.
> 
> And the kdamond can stop even after the damon_kdamond_pid() call, if the
> region_store_histogram allocation fails.  In the case, kdamond_pid will again
> be stale.  'enabled' parameter value will also be stale in the case.  The worst
> part of it is that in the case, DAMON_STAT cannot be restarted before the
> system is reboot.  The bug is similar to the one [2] that Liew found and
> working on.

The last two sentences are wrong...  In case of damon_stat(), writing 'N' to
'enabled' will just update the parameter even if damon_stop() fails.  Hence,
users can restart DAMON_STAT after 'enabled' value became stale, by writing 'N'
to 'enabled' parameter to reset the value, and then writing 'Y' to 'enabled'
parameter again, to invoke damon_start() call.  Still far from good user
experience, but less critical than the bug that Liew is working on.

> 
> I will hold this patch and fix the bug first.

My above opinion is same.  I will fix the bugs first.


Thanks,
SJ

[...]