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

[PATCH 6.18.y] 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.

 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..8217815de898 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;
+			return 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: (sashiko review) [PATCH 6.18.y] mm/damon/core: do non-safe region walk on kdamond_apply_schemes()

[SJ Park]

TL;DR: This patch is wrong, please ignore.  I will send v2.

# sashiko review suggestions
#
# 1. Consider reducing recipients.  Maybe the author,
#    maintainers, reviewers, and mailing list of the
#    direct subsystem and parent susystem mailing lists
#    could be a starting point.
# 2. Add short summary of your opinion at the beginning.
#    For example:
#
#      TL;DR: sashiko found an issue.  I will respin.
#      TL;DR: sashiko found no issue on this patch.
#
# Please DON'T FORGET removing this comment block before
# sending this!

Somehow Sashiko stopped sending review as reply, so manually sharing it.

Forwarding full sashiko review in a reply format with my 
inline comments below, for sharing details of my view and
doing followup discussions via mails if needed.

> # review url: https://sashiko.dev/#/patchset/20261003163101.32739-1-sj@kernel.org
> # start of sashiko.dev inline review
> commit 21f2d44a9b003b2e8d2502cbeb64ca223ff26e08
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: do non-safe region walk on kdamond_apply_schemes()
> 
> The commit replaces the safe region iterator damon_for_each_region_safe()
> with damon_for_each_region() in kdamond_apply_schemes(). It updates
> damos_skip_charged_region() to return true when a region is split, allowing
> the new fragment to be processed in the next iteration.
> 
> 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..8217815de8982 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;
> > +			return true;
> >  		}
> >  	}
> >  out:
> 
> [Severity: High]
> Does this early return bypass the cleanup block at the out label?
> 
> If damon_split_region_at() fails on the final region due to memory
> pressure, returning true here skips the out block that clears
> quota->charge_target_from.
> 
> Could this leaked cursor cause the scheme to permanently skip all
> subsequent memory targets in this and future evaluation intervals until
> an allocation succeeds?

Correct.  It is my mistake during porting.  I will fix this in the next
version.

> 
> [ ... ]
> 
> > @@ -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]
> Can changing this to a dynamic list iterator lead to regions being
> processed multiple times?
> 
> If a DAMON configuration has multiple schemes, and a later scheme splits
> a region, the newly inserted right-hand fragment will be visited by this
> outer loop in the next iteration.
> 
> Since damon_do_apply_schemes() evaluates all schemes sequentially,
> wouldn't schemes that already processed the original uncut region
> process the new fragment again, leading to duplicate memory operations
> and double-counting in quota tracking?

Good finding.  The bug exist in mainline too, and doesn't cause critical crash
or corruption.  Hence I don't think this is a blocker of this patch.  I will
separately work on it.  Please let me know if anyone has a different opinion.


> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20261003163101.32739-1-sj@kernel.org

# hkml [1] generated a draft of this mail.  You can regenerate
# this using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20261003163101.32739-1-sj@kernel.org
#
# [1] https://github.com/sjp38/hackermail


Thanks,
SJ

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm: damon: state leak in damos_skip_charged_region() on split failure
- [High] mm: damon: double processing of regions due to dynamic iteration
--

commit 21f2d44a9b003b2e8d2502cbeb64ca223ff26e08
Author: SeongJae Park <sj@kernel.org>

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

The commit replaces the safe region iterator damon_for_each_region_safe()
with damon_for_each_region() in kdamond_apply_schemes(). It updates
damos_skip_charged_region() to return true when a region is split, allowing
the new fragment to be processed in the next iteration.

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..8217815de8982 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;
> +			return true;
>  		}
>  	}
>  out:

[Severity: High]
Does this early return bypass the cleanup block at the out label?

If damon_split_region_at() fails on the final region due to memory
pressure, returning true here skips the out block that clears
quota->charge_target_from.

Could this leaked cursor cause the scheme to permanently skip all
subsequent memory targets in this and future evaluation intervals until
an allocation succeeds?

[ ... ]

> @@ -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]
Can changing this to a dynamic list iterator lead to regions being
processed multiple times?

If a DAMON configuration has multiple schemes, and a later scheme splits
a region, the newly inserted right-hand fragment will be visited by this
outer loop in the next iteration.

Since damon_do_apply_schemes() evaluates all schemes sequentially,
wouldn't schemes that already processed the original uncut region
process the new fragment again, leading to duplicate memory operations
and double-counting in quota tracking?

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

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

[SJ Park]

On Sat, 03 Oct 2026 17:56:23 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] mm: damon: state leak in damos_skip_charged_region() on split failure
> - [High] mm: damon: double processing of regions due to dynamic iteration

I will fix the state leak.  The second one is a minor bug that also exist in
mainline, so no blocker of this patch in my opinion.  Please read my previous
reply [1] for more details.

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


Thanks,
SJ

[...]