FAILED: patch "[PATCH] mm/damon/core: fix unconditionally skip last region" failed to apply to 6.12-stable tree

From: SeongJae Park <[email protected]>

[ Upstream commit dfb1b0c9dc0d61e422905640e1e7334b3cf6f384 ]

The macro is for the default minimum size of each DAMON region. There was a case that a reader was confused if it is the minimum number of total DAMON regions, which is set on damon_attrs->min_nr_regions. Make the name more explicit.

Link https://lkml.kernel.org/r/[email protected] open here

Signed-off-by SeongJae Park <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Stable backport preparation for b3723b596b548 ("mm/damon/core: fix unconditionally skip last region"):

Rename all uses present in 6.12, including the core KUnit override and tests. Drop changes to address-unit/minimum-size context fields and the four-argument damon_set_regions() calls, which do not exist in this tree.

Match the target's quota helper preimage by passing the fixed region size as min_region_sz and incorporating the existing-function changes from 1745ccbd2907 ("mm/damon/core: do non-safe region walk on kdamond_apply_schemes()"). The region walk must visit newly split tails when the helper returns true for the already charged prefix. Preserve the existing target-validity check and leave the last-region fix to the target commit. No new functions or context fields are introduced.

[ sashal: Reduced backport -- upstream dfb1b0c9dc0d6 touches 7 file(s), this
backport carries 4. Not backported here:
mm/damon/lru_sort.c
mm/damon/reclaim.c
mm/damon/sysfs.c
mm/damon/tests/vaddr-kunit.h
This note is generated from the file lists only; see the resolution record for the reasoning. ]

Stable-dep-of: b3723b596b54 ("mm/damon/core: fix unconditionally skip last region")

Signed-off-by Sasha Levin <[email protected]>

---

mm/damon/core.c +1 -1

@@ -1339,15 +1342,14 @@ static bool damos_skip_charged_region(struct damon_target *t,
 		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);
+					r->ar.start, min_region_sz);
 			if (!sz_to_skip) {

I will post a manual backport. Could we drop this series and use the manual backport instead?

From: Liew Rui Yan <[email protected]>

[ Upstream commit b3723b596b548c837a766aae3553c14a7b15af2b ]

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/[email protected] open here

Fixes 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")

Signed-off-by Liew Rui Yan <[email protected]>

Reviewed-by SJ Park <[email protected]>

Signed-off-by SJ Park <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Cc <[email protected]> # v5.16.x

Signed-off-by Sasha Levin <[email protected]>

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

mm/damon/core.c +14 -11

index 53d479da2007e..2a24e817e5360 100644
@@ -1325,36 +1325,39 @@ static bool damos_skip_charged_region(struct damon_target *t,
 {
 	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, min_region_sz);
 			if (!sz_to_skip) {
-				if (damon_sz_region(r) <= min_region_sz)
-					return true;
+				if (damon_sz_region(r) <= min_region_sz) {
+					skip = true;
+					goto out;
+				}
 				sz_to_skip = min_region_sz;
 			}
 			damon_split_region_at(t, r, sz_to_skip);
-			return true;
+			skip = true;
 		}
+	}
+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,

Signature · 1 line

2.53.0

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 <[email protected]> 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/[email protected] open here

Quoted 4 lines

mm/damon/core.c +0 -0

index 32737cc5f877b..802005d9186e8 100644

[ ... ]

mm/damon/core.c static bool damos_skip_charged_region(struct damon_target *t, · line 1348

Quoted 4 lines

mm/damon/core.c +0 -0

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

mm/damon/core.c static bool damos_skip_charged_region(struct damon_target *t,

Quoted 6 lines

mm/damon/core.c +4 -0

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

Quoted 7 lines

mm/damon/core.c +1 -1

 		quota->charge_target_from = NULL;
 		quota->charge_addr_from = 0;
 	}
-	return false;
+	return skip;
 }

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

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 <[email protected]>

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/[email protected] open here

Quoted 4 lines

mm/damon/core.c +0 -0

index 32737cc5f877b..cc7250f0936af 100644

[ ... ]

Quoted 34 lines

mm/damon/core.c +9 -9

@@ -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;
+				}

mm/damon/core.c static bool damos_skip_charged_region(struct damon_target *t, · line 1355

Quoted 10 lines

mm/damon/core.c +4 -0

 				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?

Quoted 9 lines

mm/damon/core.c +1 -1

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

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: damon: double application of DAMOS schemes due to non-safe region walk --

commit 7c60a2f66acad38f06b09b2a9e484519168800bc
Author: SeongJae Park <[email protected]>

mm/damon/core: do non-safe region walk on kdamond_apply_schemes()

This commit changes kdamond_apply_schemes() to use the non-safe damon_for_each_region() iterator instead of damon_for_each_region_safe(). It also drops the double-pointer workaround in damos_skip_charged_region() to let newly split regions be seen on the next iteration of the region walk.

Link https://lkml.kernel.org/r/[email protected] open here

mm/damon/core.c static void kdamond_apply_schemes(struct damon_ctx *c) · line 1754

Quoted 12 lines

mm/damon/core.c +1 -1

index 32737cc5f877b..76f23c1d2f56b 100644
@@ -1749,7 +1749,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
 		return;
 
 	damon_for_each_target(t, c) {
-		damon_for_each_region_safe(r, next_r, t)
+		damon_for_each_region(r, t)
 			damon_do_apply_schemes(c, t, r);
 	}

[Severity: High]
Does this change cause earlier schemes to double-apply their actions when a subsequent scheme splits the region?

In kdamond_apply_schemes(), changing to the non-safe iterator means the loop dynamically visits newly created fragments on the very next iteration.

When damon_do_apply_schemes() runs, it evaluates all schemes sequentially on the region:

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

If an earlier scheme evaluates the region, it may apply its action and update its statistics for the full size of the region.

If a subsequent scheme then evaluates the same region and splits it (for example via damos_skip_charged_region()), it shrinks the original region and inserts a new region immediately after it.

Because damon_for_each_region() is no longer safe against modifications to the list, the next iteration of the outer loop will process the newly inserted region across all schemes.

Since the newly inserted region retains the exact properties (age, access counts) of the original region, will the earlier scheme re-evaluate it and apply its action a second time, double-charging its statistics and applying redundant actions to the memory?

Signature · 2 lines

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm/damon: dangling charge_target_from pointer permanently freezes schemes --

commit a9baf64370898b2dec04fa2f17e4061166d6d27a
Author: Liew Rui Yan <[email protected]>

mm/damon/core: fix unconditionally skip last region

This commit modifies damos_skip_charged_region() to clear the quota's charge_target_from pointer only when the last region of a target is reached, rather than clearing it unconditionally during previous charge checks.

Link https://lore.kernel.org/[email protected] open here

Quoted 14 lines

mm/damon/core.c +1 -0

@@ -1323,36 +1323,39 @@ static bool damos_skip_charged_region(struct damon_target *t,
 		struct damon_region *r, struct damos *s)
 {
 	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;

[ ... ]

mm/damon/core.c static bool damos_skip_charged_region(struct damon_target *t, · line 1357

Quoted 6 lines

mm/damon/core.c +3 -0

+	}
+out:
+	if (r = damon_last_region(t)) {
 		quota->charge_target_from = NULL;
 		quota->charge_addr_from = 0;
 	}

[Severity: High]
If the target tracked by quota->charge_target_from is dynamically destroyed via sysfs, or if it loses all of its regions, will this clearing logic ever be reached?

If the saved target is removed or has zero regions, the region loop will be empty, meaning r = damon_last_region(t) is never evaluated for that target.

Since charge_target_from is never cleared in these scenarios, wouldn't this cause the condition earlier in this function:

        if (t != quota->charge_target_from)
                return true;

to remain true indefinitely for all other valid targets, permanently freezing the scheme? Additionally, if the target is freed, does this leave a dangling pointer comparison for future checks?

Quoted 3 lines

mm/damon/core.c +1 -1

-	return false;
+	return skip;
 }

Signature · 1 line

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

Patch 1 is a dependency of patch 2. Without it, patch 2 introduces out-of-bounds memory access bug that was found by Sashiko. Patch 2 fixes a bug that categorized to be backported to stable@.