Re: [RFC PATCH v2 1/2] samples/damon/mtier: handle damon_start() failure

[SeongJae Park <sj@kernel.org>]

On Tue, 09 Jun 2026 08:04:13 -0700 SeongJae Park <sj@kernel.org> wrote:

> On Tue, 09 Jun 2026 07:58:22 -0700 SeongJae Park <sj@kernel.org> wrote:
> 
> > On Tue, 09 Jun 2026 14:35:38 +0000 sashiko-bot@kernel.org wrote:
> > 
> > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > 
> > > Pre-existing issues:
> > > - [High] Resource leak (struct damon_ctx, struct damos, struct damon_target, and struct pid) on damon_start() failure.
> > > --
> > > 
> > > commit e95eb25cfbc21e60927df5e7e4eadcf5468c1c41
> > > Author: SeongJae Park <sj@kernel.org>
> > > samples/damon/mtier: handle damon_start() failure
> > > 
> > > This commit fixes a resource leak in the memory tiering DAMON sample module
> > > where failing to start the DAMON contexts leads to memory leaks and
> > > indefinitely running contexts.
> > > 
> > > Link: https://lore.kernel.org/20260608112455.274231F00893@smtp.kernel.org
> > > 
> > > > diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> > > > --- a/samples/damon/mtier.c
> > > > +++ b/samples/damon/mtier.c
> > > 
> > > [ ... ]
> > > 
> > > > @@ -185,7 +186,15 @@ static int damon_sample_mtier_start(void)
> > > >  		return -ENOMEM;
> > > >  	}
> > > >  	ctxs[1] = ctx;
> > > > -	return damon_start(ctxs, 2, true);
> > > > +	err = damon_start(ctxs, 2, true);
> > > > +	if (!err)
> > > > +		return 0;
> > > > +
> > > > +	if (damon_is_running(ctxs[0]))
> > > > +		damon_stop(ctxs, 1);
> > > > +	damon_destroy_ctx(ctxs[0]);
> > > > +	damon_destroy_ctx(ctxs[1]);
> > > > +	return err;
> > > >  }
> > > 
> > > [Severity: High]
> > > This is a pre-existing issue, but it looks like the exact same resource leak
> > > pattern exists in other DAMON sample modules when damon_start() fails.
> > > 
> > > Looking at damon_sample_prcl_start() in samples/damon/prcl.c:
> > > 
> > > 	err = damon_start(&ctx, 1, true);
> > > 	if (err)
> > > 		return err;
> > > 
> > > And similarly in damon_sample_wsse_start() in samples/damon/wsse.c:
> > > 
> > > 	err = damon_start(&ctx, 1, true);
> > > 	if (err)
> > > 		return err;
> > > 
> > > Since they return directly without calling damon_destroy_ctx() or putting
> > > the target_pidp reference, do these modules also leak their newly allocated
> > > contexts and PID references when damon_start() fails?
> > 
> > Nice finding.  Not a blocker of this patch, though.  I will separately work on
> > this.
> 
> Hmm, but...  They do destroy the ctx in damon_sample_{wsse,prcl}_stop().  And
> users cannot trigger damon_sample_{wsse,prcl}_start() without calling the stop
> function first, due to the current 'enabled' value based skippinng in
> damon_sample_wsse_enable_store().  So, this seems not a true issue.  Let me
> know if I'm missing something.

Ok, I was missing this.  damon_sample_wsse_enable_store() reset 'enabled' when
damon_sample_wsse_start() fails.  Same pattern for prcl and mtier.  So seems
the issue is real.  I will separately work on this.


Thanks,
SJ

[...]