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

[SeongJae Park <sj@kernel.org>]

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.  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.

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.

[1] http://git.kernel.org/sj/c/16a0e8ecd699f86b


Thanks,
SJ

[...]