Re: (sashiko review) [PATCH] mm/damon/stat: deallocate damon_call() failure leaking damon_ctx

[SeongJae Park]

Andrew, please drop this patch.  sashiko found an issue, so I will respin.

More details below.

Forwarding Sashiko review in the usual reply format with my inline comments,
for doing discussions via mails.

> # review url: https://sashiko.dev/#/patchset/20260402010457.66860-1-sj@xxxxxxxxxx
> # start of sashiko.dev inline review
> commit 1d53bb9291d3fde393f37b143e637f0d25b6634e
> Author: SeongJae Park <sj@xxxxxxxxxx>
> 
> mm/damon/stat: deallocate damon_call() failure leaking damon_ctx
> 
> This commit addresses a memory leak by deallocating damon_stat_context if
> damon_call() fails during stat startup.
> 
> Link: https://lore.kernel.org/20260401012428.86694-1-sj@xxxxxxxxxx
> 
> > diff --git a/mm/damon/stat.c b/mm/damon/stat.c
> > --- a/mm/damon/stat.c
> > +++ b/mm/damon/stat.c
> > @@ -257,7 +257,12 @@ static int damon_stat_start(void)
> >  
> >  	damon_stat_last_refresh_jiffies = jiffies;
> >  	call_control.data = damon_stat_context;
> > -	return damon_call(damon_stat_context, &call_control);
> > +	err = damon_call(damon_stat_context, &call_control);
> > +	if (err) {
> > +		damon_destroy_ctx(damon_stat_context);
> 
> Can this cause a use-after-free?
> 
> Earlier in damon_stat_start(), damon_start() is called, which creates
> and starts the kdamond_fn kernel thread. This thread actively uses the
> damon_stat_context.
> 
> If damon_call() fails, the kdamond_fn thread might still be running or
> in its teardown phase. If we free the context directly using
> damon_destroy_ctx() before the kthread has fully exited, the kthread
> might access freed memory.

Nice catch.

FYI, I initially thought damon_call() of DAMON_STAT cannot fail, because it
synchronizes its damon_start()/damon_stop() calls with module parameter
handling function, and it doesn't update the context internal state, which
means the damon_ctx->maybe_corrupted cannot be set.  If that's true, this patch
itself is not needed since the memory leak cannot exist.

But, kdamond can fail for its internal memory allocation failures.
Specifically, if ctx->region_score_histogram allocation is failed, it will be
terminated.  So, yes, sashiko is right.  There is a chance.

> 
> Should we call damon_stop() here to wait for the thread to safely exit
> before destroying the context, similar to the teardown sequence in
> damon_stat_stop()?

Seems that is a workable option.  But given the fact that kdamond is already in
its termination step, it feels odd to me.  I'll take more time to think about.

> 
> > +		damon_stat_context = NULL;
> > +	}
> > +	return err;
> >  }
> 
> 
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260402010457.66860-1-sj@xxxxxxxxxx


Thanks,
SJ

[...]