Re: [PATCH 02/10] mm/damon/core: add damon_new_region() debug_sanity check

[Zenghui Yu <yuzenghui@huawei.com>]

On 2026/6/8 0:53, SeongJae Park wrote:
> Hello Zenghui,
> 
> On Sun, 7 Jun 2026 23:24:27 +0800 Zenghui Yu <zenghui.yu@linux.dev> wrote:
> 
> > Hi SeongJae,
> >
> > On 3/6/26 11:29 PM, SeongJae Park wrote:
> > > damon_new_region() is supposed to be called with only valid address
> > > range arguments.  Do the check under DAMON_DEBUG_SANITY.
> > >
> > > Signed-off-by: SeongJae Park <sj@kernel.org>
> > > ---
> > >  mm/damon/core.c | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > index f1a97e85824ac..0c1353164ec81 100644
> > > --- a/mm/damon/core.c
> > > +++ b/mm/damon/core.c
> > > @@ -109,6 +109,17 @@ int damon_select_ops(struct damon_ctx *ctx, enum damon_ops_id id)
> > >  	return err;
> > >  }
> > >  
> > > +#ifdef CONFIG_DAMON_DEBUG_SANITY
> > > +static void damon_verify_new_region(unsigned long start, unsigned long end)
> > > +{
> > > +	WARN_ONCE(start >= end, "start %lu >= end %lu\n", start, end);
> > > +}
> > > +#else
> > > +static void damon_verify_new_region(unsigned long start, unsigned long end)
> > > +{
> > > +}
> > > +#endif
> > > +
> > >  /*
> > >   * Construct a damon_region struct
> > >   *
> > > @@ -118,6 +129,7 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
> > >  {
> > >  	struct damon_region *region;
> > >  
> > > +	damon_verify_new_region(start, end);
> > >  	region = kmem_cache_alloc(damon_region_cache, GFP_KERNEL);
> > >  	if (!region)
> > >  		return NULL;
> >
> > This can be triggered with
> >
> > 	echo Y > /sys/module/damon_sample_mtier/parameters/enabled
> >
> > because both node{0,1}_{start,end}_addr are 0 if people forget to properly
> > initialize them.
> 
> Nice finding!
> 
> > This can be avoided by checking the parameters right
> > before damon_new_region(). But I'm not sure if this is the correct
> > solution.
> >
> > diff --git a/samples/damon/mtier.c b/samples/damon/mtier.c
> > index 775838a23d93..4a5d3fb12e1b 100644
> > --- a/samples/damon/mtier.c
> > +++ b/samples/damon/mtier.c
> > @@ -118,6 +118,9 @@ static struct damon_ctx *damon_sample_mtier_build_ctx(bool promote)
> >  	} else {
> >  		addr.start = promote ? node1_start_addr : node0_start_addr;
> >  		addr.end = promote ? node1_end_addr : node0_end_addr;
> > +
> > +		if (addr.start >= addr.end)
> > +			goto free_out;
> >  	}
> >  
> >  	region = damon_new_region(addr.start, addr.end);
> 
> Because mtier is just a sample module, and this doesn't cause a catastrophic
> situation like system crash, I think this is very urgent.

I assume you meant "this is not very urgent" ;) .

> But, given the
> simplicity of the change, this looks good to me.  If you'd like to send a
> patch, please feel free to do.

Sure, I will send it out shortly.

> 
> The real problem in my perspective is, however, the fact that DAMON core is
> not providing a central parameters sanity check.  As a result, each DAMON core
> API callers are implementing their own validation that often and repeatedly
> turns out to be incomplete, like this.  I'm working on such central sanity
> check and further refactoring DAMON API.  Some of work in progress [1] is
> available at damon/next tree.  While the work is ongoing, adding this kind of
> additional check should also be fine.

I do agree with that!

Thanks,
Zenghui