Re: [PATCH 6.12.y] mm/damon/core: fix unconditionally skip last region

[SJ Park <sj@kernel.org>]

On Wed, 30 Sep 2026 09:53:34 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Critical] damon: OOB write and list corruption when damon_split_region_at() fails
> - [High] damon: permanent skipping of targets due to charge_target_from state leak
> --
> 
> commit 035ce5241b6b256409f7ad3d4de6c01dbe7e8563
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> mm/damon/core: fix unconditionally skip last region
> 
> This patch modifies the skip logic in DAMON to only reset the
> charge_target_from state when the last region is reached. It also updates
> the function to skip the region only if it is applied or cannot be split,
> rather than unconditionally skipping the last region.
> 
> Link: https://lore.kernel.org/20260908134739.96919-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 32737cc5f877b..802005d9186e8 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -1348,11 +1346,15 @@ static bool damos_skip_charged_region(struct damon_target *t,
> >  			damon_split_region_at(t, r, sz_to_skip);
> >  			r = damon_next_region(r);
> >  			*rp = r;
> 
> [Severity: Critical]
> Could this lead to out-of-bounds writes and list corruption if
> damon_split_region_at() fails?
> 
> This patch removes the early exit condition for the last region, exposing it
> to damon_split_region_at(). If memory allocation fails, the split function
> returns silently without modifying the list:
> 
> mm/damon/core.c:damon_split_region_at() {
> 	new = damon_new_region(r->ar.start + sz_r, r->ar.end);
> 	if (!new)
> 		return;
> ...
> }
> 
> The pointer is then unconditionally advanced with r = damon_next_region(r).
> If r was the last region, damon_next_region() wraps around to the list
> head (&t->regions_list), generating an invalid struct damon_region pointer
> pointing outside valid regions. Subsequent writes to this pointer will
> corrupt the target sibling list integrity.

This is a pre-existing issue.  I will separately work on the fix.

> 
> > +			skip = false;
> >  		}
> > +	}
> > +out:
> > +	if (r = damon_last_region(t)) {
> 
> [Severity: High]
> Does the early return inside the region size check bypass this cleanup logic?
> 
> If sz_to_skip evaluates to 0 and the region size is <= DAMON_MIN_REGION,
> the code hits an early return:
> 
> mm/damon/core.c:damos_skip_charged_region() {
> ...
> 		sz_to_skip = ALIGN_DOWN(quota->charge_addr_from -
> 				r->ar.start, DAMON_MIN_REGION);
> 		if (!sz_to_skip) {
> 			if (damon_sz_region(r) <= DAMON_MIN_REGION)
> 				return true;
> ...
> }
> 
> Because this early return bypasses the new out label,
> quota->charge_target_from remains populated if this occurs on the last
> region. For all subsequent targets, the check
> if (t != quota->charge_target_from) evaluates to true and indefinitely skips
> those regions, causing targets to be starved.

Good catch.  This is a mistake in my backporting.  I will send v2.

> 
> >  		quota->charge_target_from = NULL;
> >  		quota->charge_addr_from = 0;
> >  	}
> > -	return false;
> > +	return skip;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260930092829.45885-1-sj@kernel.org?part=1
> 


Thanks,
SJ