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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm/damon: out-of-bounds memory access when region split fails on last region --

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

mm/damon/core: fix unconditionally skip last region

This commit fixes an issue where the last region of a target is unconditionally skipped and reset when a quota is set, even if it hasn't been processed.

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

Quoted 13 lines

mm/damon/core.c +0 -5

index 859a6a636ab0e..64ff03b15fdef 100644
@@ -768,14 +769,11 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 		if (quota->charge_target_from) {
 			if (t != quota->charge_target_from)
 				continue;
-			if (r == damon_last_region(t)) {
-				quota->charge_target_from = NULL;
-				quota->charge_addr_from = 0;
-				continue;
-			}

[ ... ]

Quoted 5 lines

mm/damon/core.c +0 -1

@@ -783,17 +781,25 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 						r->ar.start, DAMON_MIN_REGION);
 				if (!sz) {
 					if (damon_sz_region(r) <=
-					    DAMON_MIN_REGION)

mm/damon/core.c static void damon_do_apply_schemes(struct damon_ctx *c, · line 792

Quoted 10 lines

mm/damon/core.c +4 -1

-						continue;
+					    DAMON_MIN_REGION) {
+						skip = true;
+						goto clean_charge_from_skip;
+					}
 					sz = DAMON_MIN_REGION;
 				}
 				damon_split_region_at(t, r, sz);
 				r = damon_next_region(r);
 				sz = damon_sz_region(r);

[Severity: High]
Since this patch removes the early check for the last region, what happens in damon_do_apply_schemes() if damon_split_region_at() fails on the last region?

If damon_split_region_at() fails under memory pressure, it returns silently without splitting the region. The code then unconditionally calls r = damon_next_region(r).

Because r was the last region and was not split, its list.next points to the list head. damon_next_region() then returns an invalid pointer.

The next line calls damon_sz_region(r), which dereferences this invalid pointer and causes an out-of-bounds memory access.

Does this code need a check to ensure the region was successfully split before advancing the pointer?

Signature · 2 lines

-- 
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 processing and skipped regions due to broken backport --

commit 18797502eb0b5cf3a96dbf53e3e238e6ea4158ce
Author: SeongJae Park <[email protected]>

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

This commit replaces the damon_for_each_region_safe iterator with damon_for_each_region in kdamond_apply_schemes. This change ensures that newly split regions from address filters are properly processed during the iteration.

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

Quoted 4 lines

mm/damon/core.c +0 -0

index 7afbc20833ea4..c65d2cbeea6b5 100644

[ ... ]

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

Quoted 6 lines

mm/damon/core.c +1 -1

@@ -912,7 +912,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
 	}
 
 	damon_for_each_target(t, c) {
-		damon_for_each_region_safe(r, next_r, t)
+		damon_for_each_region(r, t)

[Severity: High]
Does this change result in regions being skipped and double-processed?

The commit message mentions dropping the workaround in damos_skip_charged_region(), but in this stable kernel version, that helper does not exist yet and the workaround is still inlined in damon_do_apply_schemes(). Since the inline workaround wasn't removed in this backport, it appears to conflict with the new loop iterator.

If a scheme's quota->charge_addr_from check triggers a region split in damon_do_apply_schemes(), the local pointer is advanced:

mm/damon/core.c:damon_do_apply_schemes() {
    ...
			if (quota->charge_addr_from && r->ar.start <
					quota->charge_addr_from) {
				sz = ALIGN_DOWN(quota->charge_addr_from -
						r->ar.start, DAMON_MIN_REGION);
				if (!sz) {
					if (damon_sz_region(r) <=
					    DAMON_MIN_REGION)
						continue;
					sz = DAMON_MIN_REGION;
				}
				damon_split_region_at(t, r, sz);
				r = damon_next_region(r);
				sz = damon_sz_region(r);
			}
    ...
}

Because r is reassigned locally within the inner scheme loop, subsequent schemes will apply their actions to the newly created second-half region, completely skipping the first half.

Then, when damon_do_apply_schemes() returns to the outer loop in kdamond_apply_schemes():

		damon_for_each_region(r, t)
			damon_do_apply_schemes(c, t, r);

The iterator will advance naturally to the newly split second-half region. This causes damon_do_apply_schemes() to be called a second time on the exact same region, leading to DAMON actions being applied multiple times to the same memory, double-charged quotas, and corrupted statistics.

Quoted 4 lines

mm/damon/core.c +0 -0

 			damon_do_apply_schemes(c, t, r);
 	}
 }

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/damon: Out-of-bounds access due to unchecked split failure on last region --

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

mm/damon/core: fix unconditionally skip last region

This commit modifies the quota mechanism in DAMON to only reset the charge target and address from when the last region is reached. It ensures the last region is not skipped if it has not been processed.

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

Quoted 4 lines

mm/damon/core.c +0 -0

index c65d2cbeea6b5..f2f6f125c09b1 100644

[ ... ]

Quoted 24 lines

mm/damon/core.c +4 -8

@@ -768,14 +769,11 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 		if (quota->charge_target_from) {
 			if (t != quota->charge_target_from)
 				continue;
-			if (r == damon_last_region(t)) {
-				quota->charge_target_from = NULL;
-				quota->charge_addr_from = 0;
-				continue;
-			}
 			if (quota->charge_addr_from &&
-					r->ar.end <= quota->charge_addr_from)
-				continue;
+					r->ar.end <= quota->charge_addr_from) {
+				skip = true;
+				goto clean_charge_from_skip;
+			}
 
 			if (quota->charge_addr_from && r->ar.start <
 					quota->charge_addr_from) {

@@ -783,17 +781,25 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 						r->ar.start, DAMON_MIN_REGION);
 				if (!sz) {
 					if (damon_sz_region(r) <=
-					    DAMON_MIN_REGION)

mm/damon/core.c static void damon_do_apply_schemes(struct damon_ctx *c, · line 792

Quoted 10 lines

mm/damon/core.c +4 -1

-						continue;
+					    DAMON_MIN_REGION) {
+						skip = true;
+						goto clean_charge_from_skip;
+					}
 					sz = DAMON_MIN_REGION;
 				}
 				damon_split_region_at(t, r, sz);
 				r = damon_next_region(r);
 				sz = damon_sz_region(r);

[Severity: High]
Since this commit removes the check that bypassed damon_split_region_at() for the target's last region, does this introduce an out-of-bounds access if memory allocation fails?

If the last region overlaps the charge boundary, it is now passed to damon_split_region_at(). This function performs a non-blocking memory allocation that can fail under memory pressure and return silently without splitting the region:

mm/damon/core.c:damon_split_region_at() {
    new = damon_new_region(r->ar.start + sz_r, r->ar.end);
    if (!new)
        return;
    ...
}

If the allocation fails when processing the last region, the region is not split. The code then unconditionally executes:

    r = damon_next_region(r);
    sz = damon_sz_region(r);

Because r was the last region and wasn't split, damon_next_region() reads the list.next pointer which points back to the list head:

include/linux/damon.h:damon_next_region() {
    return container_of(r->list.next, struct damon_region, list);
}

This produces an out-of-bounds pointer derived from the list head. When this pointer is immediately dereferenced by damon_sz_region(r), could this regression result in out-of-bounds memory reads?

Signature · 2 lines

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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm/damon: double-application of earlier schemes to newly split regions --

commit 01d768c4d318527360ef5913771d20f22a99984a
Author: SeongJae Park <[email protected]>

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

kdamond_apply_schemes() is using damon_for_each_region_safe(), which is safe for deallocation of the region inside the loop. However, the loop internal logic does not deallocate regions. Hence it is only wasting the next pointer.

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

Quoted 4 lines

mm/damon/core.c +0 -0

index 7afbc20833ea4..b5c85e6488596 100644

[ ... ]

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

Quoted 7 lines

mm/damon/core.c +1 -1

@@ -912,7 +911,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
 	}
 
 	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 code cause earlier schemes to be applied twice to newly split regions?

If multiple schemes are configured, an earlier scheme (e.g. Scheme 0) might apply its action to a full, un-split region. If a later scheme (e.g. Scheme 1) then decides to split the region, damon_split_region_at() inserts the newly split part into the list immediately after the current region.

Because this loop in kdamond_apply_schemes() now uses damon_for_each_region() instead of the safe variant, the outer loop will advance to this newly split region and call damon_do_apply_schemes() on it again.

Will this cause damon_do_apply_schemes() to evaluate the new region from the beginning of the scheme list, resulting in Scheme 0 executing its memory management actions (like madvise) and counting statistics a second time on the same memory range?

Quoted 3 lines

mm/damon/core.c +0 -0

 	}
 }

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: damon: use-after-free and permanent scheme disablement on target destroy --

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

mm/damon/core: fix unconditionally skip last region

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.

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

mm/damon/core.c static void damon_do_apply_schemes(struct damon_ctx *c, · line 771

Quoted 8 lines

mm/damon/core.c +0 -0

index b5c85e6488596..1b9fb00405cfb 100644
@@ -768,14 +769,11 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 		if (quota->charge_target_from) {
 			if (t != quota->charge_target_from)
 				continue;

[Severity: High]
Does this code have a use-after-free and permanent scheme disablement issue when a target is destroyed?

If the user dynamically removes a target via sysfs, damon_sysfs_set_targets() calls damon_destroy_target() which frees the target memory:

mm/damon/sysfs.c:damon_sysfs_set_targets() {
        ...
		} else {
			if (damon_target_has_pid(ctx))
				put_pid(t->pid);
			damon_destroy_target(t);
		}
        ...
}

mm/damon/core.c:damon_destroy_target() {
	damon_del_target(t);
	damon_free_target(t);
}

Existing schemes are updated in-place via damon_sysfs_update_scheme(), which does not clear the charge_target_from pointer, leaving a dangling pointer in the scheme.

Immediately after, when kdamond_apply_schemes() runs in the same thread, damon_do_apply_schemes() compares the valid remaining targets against this dangling pointer (if (t != quota->charge_target_from)). Since it does not match the remaining targets, it unconditionally executes continue, skipping the scheme for all regions.

Because a matching target is never reached, the pointer is never reset (the code cannot reach the clean_charge_from_skip label). This results in permanent disablement of the scheme and persistent use-after-free comparisons against the dangling pointer.

Quoted 13 lines

mm/damon/core.c +4 -7

-			if (r == damon_last_region(t)) {
-				quota->charge_target_from = NULL;
-				quota->charge_addr_from = 0;
-				continue;
-			}
 			if (quota->charge_addr_from &&
-					r->ar.end <= quota->charge_addr_from)
-				continue;
+					r->ar.end <= quota->charge_addr_from) {
+				skip = true;
+				goto clean_charge_from_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@.