Re: [PATCH] mm/damon/core: disallow overlapping input ranges for damon_set_regions()

[SJ Park <sj@kernel.org>]

On Fri, 03 Jul 2026 17:11:41 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

No blocker for this patch.

> 
> Pre-existing issues:
> - [High] Execution of ALIGN() and ALIGN_DOWN() macros with a non-power-of-two `min_region_sz`, violating alignment subsystem guidelines.

Wrong finding.

> - [High] TOCTOU race due to lockless concurrent reads of `sys_ctx->ops_id` and `sys_ctx->addr_unit`.

Correct finding but not a blocker of this patch.

> --
> 
> commit b68f48fc29d6aa945e1dd1ef0c694913e40d3854
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: disallow overlapping input ranges for damon_set_regions()
> 
> This commit reinstates validation in damon_set_regions() to ensure that input
> ranges are sorted and do not overlap. It checks the boundaries and returns an
> error if they are invalid, preventing unintended DAMON behavior.
> 
> Link: https://lore.kernel.org/20260630041806.151124-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index c6fc35be633b3..b2fc15a3804ff 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -418,12 +418,19 @@ int damon_set_regions(struct damon_target *t, struct damon_addr_range *ranges,
> >  {
> >  	struct damon_region *r, *next;
> >  	unsigned int i;
> > +	unsigned long last_end;
> >  	int err;
> >  
> >  	for (i = 0; i < nr_ranges; i++) {
> > -		if (ALIGN_DOWN(ranges[i].start, min_region_sz) >=
> > -				ALIGN(ranges[i].end, min_region_sz))
> > +		unsigned long start, end;
> > +
> > +		start = ALIGN_DOWN(ranges[i].start, min_region_sz);
> > +		end = ALIGN(ranges[i].end, min_region_sz);
> 
> [Severity: High]
> This is a pre-existing issue, but does this safely handle a non-power-of-two
> min_region_sz?
> 
> The ALIGN macros rely on bitwise operations that produce incorrect
> boundaries if the alignment argument isn't a power of two. A user can
> write a non-power-of-two value to addr_unit via sysfs, which sets
> min_region_sz to a non-power-of-two in damon_sysfs_apply_inputs():
> 
> mm/damon/sysfs.c:damon_sysfs_apply_inputs() {
>     ...
>     ctx->min_region_sz = max(
>         DAMON_MIN_REGION_SZ / sys_ctx->addr_unit, 1);
>     ...
> }
> 
> Even though damon_commit_ctx() eventually validates and rejects the
> configuration, these unaligned boundaries are actively used here to
> allocate and merge regions during setup. Could this lead to overlapping
> regions or division by zero before the rejection occurs?

We do the validation "after" the alignment, so no overlapping region can be
created.  Hence no division by zero.

> 
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but can a time-of-check to
> time-of-use (TOCTOU) race occur with sys_ctx->ops_id?
> 
> damon_sysfs_apply_inputs() locklessly reads sys_ctx->ops_id multiple
> times:
> 
> mm/damon/sysfs.c:damon_sysfs_apply_inputs() {
>     ...
>     err = damon_select_ops(ctx, sys_ctx->ops_id);
>     ...
>     if (sys_ctx->ops_id == DAMON_OPS_PADDR) {
>     ...
> }
> 
> Meanwhile, a concurrent sysfs write can update ops_id without acquiring
> damon_sysfs_lock:
> 
> mm/damon/sysfs.c:operations_store() {
>     ...
>     context->ops_id = ops_name->ops_id;
>     ...
> }
> 
> If ops_id is changed between the damon_select_ops() call and the
> subsequent check, could it result in a torn configuration where the
> context operates in one mode (e.g., DAMON_OPS_VADDR) but the region size
> scaling logic is applied using rules for another mode (DAMON_OPS_PADDR)?

Nice catch!  Not a blocker of this patch, though, as Sashiko also mentioned.  I
will separately work on this.
> 
> > +		if (start >= end)
> > +			return -EINVAL;
> > +		if (i > 0 && last_end > start)
> >  			return -EINVAL;
> > +		last_end = end;
> >  	}
> >  
> >  	/* Remove regions which are not in the new ranges */
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260703165610.92894-1-sj@kernel.org?part=1


Thanks,
SJ