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

[SJ Park <sj@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] mm: invalid list head dereference if damon_split_region_at() fails
> - [Medium] mm: scheme state leak due to early return for small last region
> --
> 
> commit ae1e4dc8429f6a152575d9c64a1876f9dfb3b7ad
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> 
> mm/damon/core: fix unconditionally skip last region
> 
> This commit modifies damos_skip_charged_region() to only reset the charge
> state when the last region is reached. It ensures the last region is skipped
> only when it is applied or cannot be split.
> 
> Link: https://lore.kernel.org/20260908134739.96919-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 0713fda22682c..5ede1733369f5 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> 
> [ ... ]
> 
> >  		if (quota->charge_addr_from && r->ar.start <
> >  				quota->charge_addr_from) {
> >  			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;
> 
> [Severity: Medium]
> Does this early return for a small last region bypass the state clearing at
> the relocated out label?
> 
> If damos_skip_charged_region() processes the last region in a target and it
> is smaller than DAMON_MIN_REGION, the code executes return true early. This
> bypasses the out block that clears quota->charge_target_from.
> 
> In the subsequent window, the scheme could again skip all targets until the
> saved target, evaluate its last region, and hit the same early return,
> potentially skipping targets endlessly across windows.

Good catch, I forgot converting this while backporting.  I will send v2.

> 
> >  				sz_to_skip = DAMON_MIN_REGION;
> >  			}
> >  			damon_split_region_at(t, r, sz_to_skip);
> >  			r = damon_next_region(r);
> >  			*rp = r;
> 
> [Severity: High]
> Can unconditionally advancing the region pointer after a potentially failed
> split on the last element cause list head type confusion?
> 
> When damos_skip_charged_region() processes the last region of a target and
> attempts to split it, it calls damon_split_region_at(). If the split fails
> due to a memory allocation failure, the region is not split and remains the
> last element in the list.
> 
> The subsequent call:
> r = damon_next_region(r);
> 
> will dereference r->list.next, which points to the list head
> (&t->regions_list), returning a type-confused pointer.
> 
> Passing this bogus pointer back via *rp = r means that subsequent calls to
> damos_valid_target() and damos_apply_scheme() will dereference it, which
> could lead to a heap out-of-bounds access.

Good catch, but pre-existing issue.  Maybe a fix that made upstream should be
applied here.  I will separately work on this.

> 
> > +			skip = false;
> >  		}
> > +	}
> > +out:
> > +	if (r == damon_last_region(t)) {
> >  		quota->charge_target_from = NULL;
> >  		quota->charge_addr_from = 0;
> >  	}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260930093707.48739-1-sj@kernel.org?part=1


Thanks,
SJ