[PATCH 6.18.y v2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes()

[PATCH 6.18.y v2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes()

[SJ Park]

From: SeongJae Park <sj@kernel.org>

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.  Also, it causes a problem.

When an address filter is applied, and there is a region that intersects
with the filter, the filter splits the region on the filter boundary.  The
intention is to let DAMOS apply action to only filtered-in address ranges.
However, it is using damon_for_each_region_safe(), which sets the next
region before the execution of the iteration.  Hence, the region that
split and now will be next to the previous region, is simply ignored.  As
a result, DAMOS applies the action to target regions bit slower than
expected, when the address filter is used.  Shouldn't be a big problem but
definitely better to be fixed.  damos_skip_charged_region() was working
around the issue using a double pointer hack.

Use damon_for_each_region(), which is safe for this use case.  And drop
the work around in damos_skip_charged_region().

Link: https://lkml.kernel.org/r/20260227170623.95384-3-sj@kernel.org
Signed-off-by: SeongJae Park <sj@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
(cherry picked from commit 1745ccbd2907db2bdaa843e4abccde4fdaccbe5d)
Signed-off-by: SJ Park <sj@kernel.org>
---
This patch is required to fix a bug that is introduced by commit
500c9259f5a6, which is a backport of upstream commit b3723b596b54
("mm/damon/core: fix unconditionally skip last region").  The backport
allowed trying damon_split_region_at() of last region in
damon_skip_charged_region().  If damon_split_region() fails, following
line in damon_skip_charged_region() assigns wrong region pointer to the
variable 'r', and propagate that via 'rp' double pointer.  As a result,
silent memory corruption can happen.  damon_split_region_at() will fail
only under extremem memory pressure, but still theoretically could
happen.  This backport fix it by removing the damon_next_region() call.

Changes from v1
- v1: https://lore.kernel.org/20261003163101.32739-1-sj@kernel.org
- Fix wrong return skipping cleanup in damos_skip_charge_region()

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

diff --git a/mm/damon/core.c b/mm/damon/core.c
index c0dcb190b06e..02cc99aa6d3e 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -1734,16 +1734,18 @@ static bool damos_valid_target(struct damon_ctx *c, struct damon_target *t,
  * This function checks if a given region should be skipped or not for the
  * reason.  If only the starting part of the region has previously charged,
  * this function splits the region into two so that the second one covers the
- * area that not charged in the previous charge widnow and saves the second
- * region in *rp and returns false, so that the caller can apply DAMON action
- * to the second one.
+ * area that not charged in the previous charge widnow, and return true.  The
+ * caller can see the second one on the next iteration of the region walk.
+ * Note that this means the caller should use damon_for_each_region() instead
+ * of damon_for_each_region_safe().  If damon_for_each_region_safe() is used,
+ * the second region will just be ignored.
  *
- * Return: true if the region should be entirely skipped, false otherwise.
+ * Return: true if the region should be skipped, false otherwise.
  */
 static bool damos_skip_charged_region(struct damon_target *t,
-		struct damon_region **rp, struct damos *s, unsigned long min_sz_region)
+		struct damon_region *r, struct damos *s,
+		unsigned long min_sz_region)
 {
-	struct damon_region *r = *rp;
 	struct damos_quota *quota = &s->quota;
 	unsigned long sz_to_skip;
 	bool skip = false;
@@ -1770,9 +1772,7 @@ static bool damos_skip_charged_region(struct damon_target *t,
 				sz_to_skip = min_sz_region;
 			}
 			damon_split_region_at(t, r, sz_to_skip);
-			r = damon_next_region(r);
-			*rp = r;
-			skip = false;
+			skip = true;
 		}
 	}
 out:
@@ -2034,7 +2034,7 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 		if (quota->esz && quota->charged_sz >= quota->esz)
 			continue;
 
-		if (damos_skip_charged_region(t, &r, s, c->min_sz_region))
+		if (damos_skip_charged_region(t, r, s, c->min_sz_region))
 			continue;
 
 		if (!damos_valid_target(c, t, r, s))
@@ -2306,7 +2306,7 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
 static void kdamond_apply_schemes(struct damon_ctx *c)
 {
 	struct damon_target *t;
-	struct damon_region *r, *next_r;
+	struct damon_region *r;
 	struct damos *s;
 	unsigned long sample_interval = c->attrs.sample_interval ?
 		c->attrs.sample_interval : 1;
@@ -2329,7 +2329,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
 
 	mutex_lock(&c->walk_control_lock);
 	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);
 	}
 
-- 
2.47.3

Re: [PATCH 6.18.y v2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes()

[sashiko-bot]

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

commit 5f2353c7bc7cba2be5c7364e80d79941a4bb1709
Author: SeongJae Park <sj@kernel.org>
Date:   Sun Oct 4 15:17:30 2026 +0000

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

This commit replaces damon_for_each_region_safe() with damon_for_each_region()
in kdamond_apply_schemes() so that regions dynamically split by scheme address
filters are not skipped. It also removes the workaround in
damos_skip_charged_region() that adjusted the region pointer, as the new
iteration logic will naturally visit the newly split regions.

Link: https://lkml.kernel.org/r/20260227170623.95384-3-sj@kernel.org

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index c0dcb190b06e3..02cc99aa6d3eb 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -1770,9 +1772,7 @@ static bool damos_skip_charged_region(struct damon_target *t,
>  				sz_to_skip = min_sz_region;
>  			}
>  			damon_split_region_at(t, r, sz_to_skip);
> -			r = damon_next_region(r);
> -			*rp = r;
> -			skip = false;
> +			skip = true;

[Severity: High]
When damon_split_region_at() splits this region, the remainder is inserted
directly after 'r'. Will this cause earlier schemes to process the remainder
a second time?

Because kdamond_apply_schemes() now uses damon_for_each_region(), the loop
will step into the newly split remainder region next. However,
damon_do_apply_schemes() iterates all schemes from the beginning for each
region. If Scheme 0 already processed the full region before Scheme 1 split
it here, Scheme 0 will now evaluate and charge the remainder portion a
second time.

[ ... ]

> @@ -2329,7 +2329,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
>  
>  	mutex_lock(&c->walk_control_lock);
>  	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 non-safe iteration lead to double-charging quotas across schemes?

If a later scheme in damon_do_apply_schemes() splits the region (either due
to skip boundaries in damos_skip_charged_region() or quota limits in
damos_apply_scheme()), the new right-hand region is immediately visited on
the next iteration of this outer loop. 

Since damon_do_apply_schemes() always begins its scheme evaluation from the
first scheme, any schemes evaluated before the split will process the newly
split right-hand region again, incorrectly double-applying physical actions
and double-charging their quotas.

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

Re: [PATCH 6.18.y v2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes()

[Sasha Levin]

> Use damon_for_each_region(), which is safe for this use case.  And drop
> the work around in damos_skip_charged_region().

Queued for 6.18, thanks.

-- 
Thanks,
Sasha