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

[sashiko-bot@kernel.org]

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.

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

>  		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