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

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

[SJ Park]

From: Liew Rui Yan <aethernet65535@gmail.com>

Once quota set, the charge_{target,addr}_from unconditionally skips and
resets at the last region of the tracked target, so the last region can be
skipped even when it has not been processed.

Example:

    1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
    2. Quota is configured to process only 100 bytes per window.
    3. Window 1: Processes R1 (0-100).  Quota is full.  charge_{target,
       addr}_from is saved at (Target, 100).
    4. Window 2: The loop reaches R2.  Because R2 is
       damon_last_region(t), the old code unconditionally returns true,
       skipping R2 entirely and resetting the charge_{target,addr}_from.

    Result: R2 is permanently skipped even though it has never been
    processed.

However, it is important to note that this is a very minor issue.  This is
because it is triggered only when the previous window saved/kept
charge_{target,addr}_from, and in the next window, all regions except the
last region were skipped by damos_skip_charged_region().

Fix this by only resetting the charge_{target,addr}_from when last region
is reached, only skipping when it is applied or cannot split.

Link: https://lore.kernel.org/20260908134739.96919-1-sj@kernel.org
Fixes: 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
Reviewed-by: SJ Park <sj@kernel.org>
Signed-off-by: SJ Park <sj@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Cc: <stable@vger.kernel.org> # v5.16.x
(cherry picked from commit b3723b596b548c837a766aae3553c14a7b15af2b)
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

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
@@ -1322,19 +1322,17 @@ static bool damos_skip_charged_region(struct damon_target *t,
 	struct damon_region *r = *rp;
 	struct damos_quota *quota = &s->quota;
 	unsigned long sz_to_skip;
+	bool skip = false;
 
 	/* Skip previously charged regions */
 	if (quota->charge_target_from) {
 		if (t != quota->charge_target_from)
 			return true;
-		if (r == damon_last_region(t)) {
-			quota->charge_target_from = NULL;
-			quota->charge_addr_from = 0;
-			return true;
-		}
 		if (quota->charge_addr_from &&
-				r->ar.end <= quota->charge_addr_from)
-			return true;
+				r->ar.end <= quota->charge_addr_from) {
+			skip = true;
+			goto out;
+		}
 
 		if (quota->charge_addr_from && r->ar.start <
 				quota->charge_addr_from) {
@@ -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;
+			skip = false;
 		}
+	}
+out:
+	if (r == damon_last_region(t)) {
 		quota->charge_target_from = NULL;
 		quota->charge_addr_from = 0;
 	}
-	return false;
+	return skip;
 }
 
 static void damos_update_stat(struct damos *s,
-- 
2.47.3

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

[sashiko-bot]

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

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

[SJ Park]

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

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

[SJ Park]

Hello stable team,


Please ignore this patch, I made a mistake while backporting.  I will send v2.
Sashiko found the mistake.  Please refer to my reply [1] to Sashiko for more
details.

[1] https://lore.kernel.org/20260930100106.52943-1-sj@kernel.org


Thanks,
SJ

[...]

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

[SJ Park]

From: Liew Rui Yan <aethernet65535@gmail.com>

Once quota set, the charge_{target,addr}_from unconditionally skips and
resets at the last region of the tracked target, so the last region can be
skipped even when it has not been processed.

Example:

    1. Target has 2 regions: R1 (0-100 bytes) and R2 (100-200 bytes).
    2. Quota is configured to process only 100 bytes per window.
    3. Window 1: Processes R1 (0-100).  Quota is full.  charge_{target,
       addr}_from is saved at (Target, 100).
    4. Window 2: The loop reaches R2.  Because R2 is
       damon_last_region(t), the old code unconditionally returns true,
       skipping R2 entirely and resetting the charge_{target,addr}_from.

    Result: R2 is permanently skipped even though it has never been
    processed.

However, it is important to note that this is a very minor issue.  This is
because it is triggered only when the previous window saved/kept
charge_{target,addr}_from, and in the next window, all regions except the
last region were skipped by damos_skip_charged_region().

Fix this by only resetting the charge_{target,addr}_from when last region
is reached, only skipping when it is applied or cannot split.

Link: https://lore.kernel.org/20260908134739.96919-1-sj@kernel.org
Fixes: 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
Reviewed-by: SJ Park <sj@kernel.org>
Signed-off-by: SJ Park <sj@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Cc: <stable@vger.kernel.org> # v5.16.x
(cherry picked from commit b3723b596b548c837a766aae3553c14a7b15af2b)
Signed-off-by: SJ Park <sj@kernel.org>
---
Changes from v1
- v1: https://lore.kernel.org/20260930092829.45885-1-sj@kernel.org
- Add missed change for damon_sz_region(r) <= min_region_sz case.

 mm/damon/core.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 32737cc5f877b..cc7250f0936af 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1322,37 +1322,41 @@ static bool damos_skip_charged_region(struct damon_target *t,
 	struct damon_region *r = *rp;
 	struct damos_quota *quota = &s->quota;
 	unsigned long sz_to_skip;
+	bool skip = false;
 
 	/* Skip previously charged regions */
 	if (quota->charge_target_from) {
 		if (t != quota->charge_target_from)
 			return true;
-		if (r == damon_last_region(t)) {
-			quota->charge_target_from = NULL;
-			quota->charge_addr_from = 0;
-			return true;
-		}
 		if (quota->charge_addr_from &&
-				r->ar.end <= quota->charge_addr_from)
-			return true;
+				r->ar.end <= quota->charge_addr_from) {
+			skip = true;
+			goto out;
+		}
 
 		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;
+				if (damon_sz_region(r) <= DAMON_MIN_REGION) {
+					skip = true;
+					goto out;
+				}
 				sz_to_skip = DAMON_MIN_REGION;
 			}
 			damon_split_region_at(t, r, sz_to_skip);
 			r = damon_next_region(r);
 			*rp = r;
+			skip = false;
 		}
+	}
+out:
+	if (r == damon_last_region(t)) {
 		quota->charge_target_from = NULL;
 		quota->charge_addr_from = 0;
 	}
-	return false;
+	return skip;
 }
 
 static void damos_update_stat(struct damos *s,
-- 
2.47.3

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: OOB dereference in damon_do_apply_schemes() on split failure
--

commit 61e0ff2e7385bb544355d1f3aa3598f7f8e9e1d0
Author: Liew Rui Yan <aethernet65535@gmail.com>

mm/damon/core: fix unconditionally skip last region

This commit changes the logic in damos_skip_charged_region() to only reset
charge_{target,addr}_from when the last region is reached. This is intended to
ensure the last region is not skipped if it has not been fully processed.

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..cc7250f0936af 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -1322,37 +1322,41 @@ static bool damos_skip_charged_region(struct damon_target *t,
>  	struct damon_region *r = *rp;
>  	struct damos_quota *quota = &s->quota;
>  	unsigned long sz_to_skip;
> +	bool skip = false;
>  
>  	/* Skip previously charged regions */
>  	if (quota->charge_target_from) {
>  		if (t != quota->charge_target_from)
>  			return true;
> -		if (r == damon_last_region(t)) {
> -			quota->charge_target_from = NULL;
> -			quota->charge_addr_from = 0;
> -			return true;
> -		}
>  		if (quota->charge_addr_from &&
> -				r->ar.end <= quota->charge_addr_from)
> -			return true;
> +				r->ar.end <= quota->charge_addr_from) {
> +			skip = true;
> +			goto out;
> +		}
>  
>  		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;
> +				if (damon_sz_region(r) <= DAMON_MIN_REGION) {
> +					skip = true;
> +					goto out;
> +				}
>  				sz_to_skip = DAMON_MIN_REGION;
>  			}
>  			damon_split_region_at(t, r, sz_to_skip);
>  			r = damon_next_region(r);
>  			*rp = r;
> +			skip = false;
>  		}
> +	}
> +out:
> +	if (r == damon_last_region(t)) {

[Severity: High]
If damon_split_region_at() fails to allocate memory, it returns silently and
the region remains unmodified. If r happens to be the last region in the list,
could damon_next_region(r) evaluate past the list boundary and return an
invalid pointer?

Since this invalid pointer is assigned to *rp and the function continues to
the out label, could it fail this (r == damon_last_region(t)) check, bypass
the intended quota reset, and return false?

If so, could the caller damon_do_apply_schemes() then receive this invalid
pointer and pass it to damos_valid_target():

mm/damon/core.c:damon_do_apply_schemes() {
    ...
		if (damos_skip_charged_region(t, &r, s))
			continue;

		if (!damos_valid_target(c, t, r, s))
			continue;
    ...
}

causing it to dereference garbage data (such as r->ar.start and r->ar.end) and
potentially lead to an out-of-bounds memory access?

>  		quota->charge_target_from = NULL;
>  		quota->charge_addr_from = 0;
>  	}
> -	return false;
> +	return skip;
>  }
>  
>  static void damos_update_stat(struct damos *s,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930101459.56041-1-sj@kernel.org?part=1

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

[SJ Park]

On Wed, 30 Sep 2026 10:26:49 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] mm: OOB dereference in damon_do_apply_schemes() on split failure
> --
> 
> commit 61e0ff2e7385bb544355d1f3aa3598f7f8e9e1d0
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> 
> mm/damon/core: fix unconditionally skip last region
> 
> This commit changes the logic in damos_skip_charged_region() to only reset
> charge_{target,addr}_from when the last region is reached. This is intended to
> ensure the last region is not skipped if it has not been fully processed.
> 
> 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..cc7250f0936af 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -1322,37 +1322,41 @@ static bool damos_skip_charged_region(struct damon_target *t,
> >  	struct damon_region *r = *rp;
> >  	struct damos_quota *quota = &s->quota;
> >  	unsigned long sz_to_skip;
> > +	bool skip = false;
> >  
> >  	/* Skip previously charged regions */
> >  	if (quota->charge_target_from) {
> >  		if (t != quota->charge_target_from)
> >  			return true;
> > -		if (r == damon_last_region(t)) {
> > -			quota->charge_target_from = NULL;
> > -			quota->charge_addr_from = 0;
> > -			return true;
> > -		}
> >  		if (quota->charge_addr_from &&
> > -				r->ar.end <= quota->charge_addr_from)
> > -			return true;
> > +				r->ar.end <= quota->charge_addr_from) {
> > +			skip = true;
> > +			goto out;
> > +		}
> >  
> >  		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;
> > +				if (damon_sz_region(r) <= DAMON_MIN_REGION) {
> > +					skip = true;
> > +					goto out;
> > +				}
> >  				sz_to_skip = DAMON_MIN_REGION;
> >  			}
> >  			damon_split_region_at(t, r, sz_to_skip);
> >  			r = damon_next_region(r);
> >  			*rp = r;
> > +			skip = false;
> >  		}
> > +	}
> > +out:
> > +	if (r == damon_last_region(t)) {
> 
> [Severity: High]
> If damon_split_region_at() fails to allocate memory, it returns silently and
> the region remains unmodified. If r happens to be the last region in the list,
> could damon_next_region(r) evaluate past the list boundary and return an
> invalid pointer?
> 
> Since this invalid pointer is assigned to *rp and the function continues to
> the out label, could it fail this (r == damon_last_region(t)) check, bypass
> the intended quota reset, and return false?
> 
> If so, could the caller damon_do_apply_schemes() then receive this invalid
> pointer and pass it to damos_valid_target():
> 
> mm/damon/core.c:damon_do_apply_schemes() {
>     ...
> 		if (damos_skip_charged_region(t, &r, s))
> 			continue;
> 
> 		if (!damos_valid_target(c, t, r, s))
> 			continue;
>     ...
> }
> 
> causing it to dereference garbage data (such as r->ar.start and r->ar.end) and
> potentially lead to an out-of-bounds memory access?

I will separately work on this.  Refer to my other reply to same issue report
for details.

[1] https://lore.kernel.org/20260930102333.58847-1-sj@kernel.org

> 
> >  		quota->charge_target_from = NULL;
> >  		quota->charge_addr_from = 0;
> >  	}
> > -	return false;
> > +	return skip;
> >  }
> >  
> >  static void damos_update_stat(struct damos *s,
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260930101459.56041-1-sj@kernel.org?part=1


Thanks,
SJ

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

[Sasha Levin]

> Fix this by only resetting the charge_{target,addr}_from when last region
> is reached, only skipping when it is applied or cannot split.

I'll hold this until the damon_split_region_at() failure handling fix
you mentioned is posted, and then take 6.12, 6.6 and 6.1 together with
it.

The 6.18 backport that is already queued has the same split failure
exposure, so it will need that fix too.

-- 
Thanks,
Sasha