damon.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH 6.1.y] mm/damon/core: fix unconditionally skip last region
       [not found] <2026092949-surpass-thimble-02a8@gregkh>
@ 2026-09-30  9:48 ` SJ Park
  2026-09-30 10:05   ` sashiko-bot
  2026-10-02 14:19   ` Sasha Levin
  2026-10-09 14:48 ` [PATCH 6.1.y v2 0/2] mm/damon/core: fix last region handling of damos_skip_charged_regions() SJ Park
  1 sibling, 2 replies; 11+ messages in thread
From: SJ Park @ 2026-09-30  9:48 UTC (permalink / raw)
  To: stable; +Cc: damon, Liew Rui Yan, SJ Park, Andrew Morton

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 | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 859a6a636ab0e..64ff03b15fdef 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -756,6 +756,7 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 		unsigned long sz = damon_sz_region(r);
 		struct timespec64 begin, end;
 		unsigned long sz_applied = 0;
+		bool skip = false;
 
 		if (!s->wmarks.activated)
 			continue;
@@ -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)
-						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);
+				skip = false;
 			}
+		}
+clean_charge_from_skip:
+		if (r == damon_last_region(t)) {
 			quota->charge_target_from = NULL;
 			quota->charge_addr_from = 0;
 		}
+		if (skip)
+			continue;
 
 		if (!damos_valid_target(c, t, r, s))
 			continue;
-- 
2.47.3


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 6.1.y] mm/damon/core: fix unconditionally skip last region
  2026-09-30  9:48 ` [PATCH 6.1.y] mm/damon/core: fix unconditionally skip last region SJ Park
@ 2026-09-30 10:05   ` sashiko-bot
  2026-09-30 10:23     ` SJ Park
  2026-10-02 14:19   ` Sasha Levin
  1 sibling, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-09-30 10:05 UTC (permalink / raw)
  To: SJ Park; +Cc: damon

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 <aethernet65535@gmail.com>

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/20260908134739.96919-1-sj@kernel.org

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 859a6a636ab0e..64ff03b15fdef 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -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;
> -			}

[ ... ]

> @@ -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)
> -						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?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 6.1.y] mm/damon/core: fix unconditionally skip last region
  2026-09-30 10:05   ` sashiko-bot
@ 2026-09-30 10:23     ` SJ Park
  0 siblings, 0 replies; 11+ messages in thread
From: SJ Park @ 2026-09-30 10:23 UTC (permalink / raw)
  To: sashiko-bot; +Cc: SJ Park, damon

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

> 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 <aethernet65535@gmail.com>
> 
> 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/20260908134739.96919-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 859a6a636ab0e..64ff03b15fdef 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -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;
> > -			}
> 
> [ ... ]
> 
> > @@ -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)
> > -						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?

The out-of-bounds memory access is indeed occurs due to this patch.  But the
lack of damon_split_region_at() error handling was anyway a bug.  So, in a
sense, pre-existing issue.  I will work on fixing it separately.

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


Thanks,
SJ

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 6.1.y] mm/damon/core: fix unconditionally skip last region
  2026-09-30  9:48 ` [PATCH 6.1.y] mm/damon/core: fix unconditionally skip last region SJ Park
  2026-09-30 10:05   ` sashiko-bot
@ 2026-10-02 14:19   ` Sasha Levin
  1 sibling, 0 replies; 11+ messages in thread
From: Sasha Levin @ 2026-10-02 14:19 UTC (permalink / raw)
  To: stable; +Cc: Sasha Levin, damon, Liew Rui Yan, SJ Park, Andrew Morton

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

-- 
Thanks,
Sasha

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 6.1.y v2 0/2] mm/damon/core: fix last region handling of damos_skip_charged_regions()
       [not found] <2026092949-surpass-thimble-02a8@gregkh>
  2026-09-30  9:48 ` [PATCH 6.1.y] mm/damon/core: fix unconditionally skip last region SJ Park
@ 2026-10-09 14:48 ` SJ Park
  2026-10-09 14:48   ` [PATCH 6.1.y v2 1/2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes() SJ Park
  2026-10-09 14:48   ` [PATCH 6.1.y v2 2/2] mm/damon/core: fix unconditionally skip last region SJ Park
  1 sibling, 2 replies; 11+ messages in thread
From: SJ Park @ 2026-10-09 14:48 UTC (permalink / raw)
  To: stable; +Cc: damon, SJ Park

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

Changes from v1
- v1: https://lore.kernel.org/20260930094853.52736-1-sj@kernel.org
- Fix out-of-bounds memory access bug by adding patch 1.

Liew Rui Yan (1):
  mm/damon/core: fix unconditionally skip last region

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

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


base-commit: af13aea0793685297faa77d75b825f0e5a99319c
-- 
2.47.3

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 6.1.y v2 1/2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes()
  2026-10-09 14:48 ` [PATCH 6.1.y v2 0/2] mm/damon/core: fix last region handling of damos_skip_charged_regions() SJ Park
@ 2026-10-09 14:48   ` SJ Park
  2026-10-09 14:56     ` sashiko-bot
  2026-10-09 14:48   ` [PATCH 6.1.y v2 2/2] mm/damon/core: fix unconditionally skip last region SJ Park
  1 sibling, 1 reply; 11+ messages in thread
From: SJ Park @ 2026-10-09 14:48 UTC (permalink / raw)
  To: stable; +Cc: damon, SeongJae Park, Andrew Morton

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>
---
 mm/damon/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 7afbc20833ea..c65d2cbeea6b 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -858,7 +858,7 @@ static void damos_set_effective_quota(struct damos_quota *quota)
 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;
 
 	damon_for_each_scheme(s, c) {
@@ -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)
 			damon_do_apply_schemes(c, t, r);
 	}
 }
-- 
2.47.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 6.1.y v2 2/2] mm/damon/core: fix unconditionally skip last region
  2026-10-09 14:48 ` [PATCH 6.1.y v2 0/2] mm/damon/core: fix last region handling of damos_skip_charged_regions() SJ Park
  2026-10-09 14:48   ` [PATCH 6.1.y v2 1/2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes() SJ Park
@ 2026-10-09 14:48   ` SJ Park
  2026-10-09 15:04     ` sashiko-bot
  1 sibling, 1 reply; 11+ messages in thread
From: SJ Park @ 2026-10-09 14:48 UTC (permalink / raw)
  To: stable; +Cc: damon, Liew Rui Yan, SJ Park, Andrew Morton

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 | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index c65d2cbeea6b..f2f6f125c09b 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -756,6 +756,7 @@ static void damon_do_apply_schemes(struct damon_ctx *c,
 		unsigned long sz = damon_sz_region(r);
 		struct timespec64 begin, end;
 		unsigned long sz_applied = 0;
+		bool skip = false;
 
 		if (!s->wmarks.activated)
 			continue;
@@ -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)
-						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);
+				skip = false;
 			}
+		}
+clean_charge_from_skip:
+		if (r == damon_last_region(t)) {
 			quota->charge_target_from = NULL;
 			quota->charge_addr_from = 0;
 		}
+		if (skip)
+			continue;
 
 		if (!damos_valid_target(c, t, r, s))
 			continue;
-- 
2.47.3

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 6.1.y v2 1/2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes()
  2026-10-09 14:48   ` [PATCH 6.1.y v2 1/2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes() SJ Park
@ 2026-10-09 14:56     ` sashiko-bot
  2026-10-09 15:09       ` SJ Park
  0 siblings, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-10-09 14:56 UTC (permalink / raw)
  To: SJ Park; +Cc: damon

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 <sj@kernel.org>

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/20260227170623.95384-3-sj@kernel.org

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 7afbc20833ea4..c65d2cbeea6b5 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -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.

>  			damon_do_apply_schemes(c, t, r);
>  	}
>  }

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 6.1.y v2 2/2] mm/damon/core: fix unconditionally skip last region
  2026-10-09 14:48   ` [PATCH 6.1.y v2 2/2] mm/damon/core: fix unconditionally skip last region SJ Park
@ 2026-10-09 15:04     ` sashiko-bot
  2026-10-09 15:11       ` SJ Park
  0 siblings, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-10-09 15:04 UTC (permalink / raw)
  To: SJ Park; +Cc: damon

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 <aethernet65535@gmail.com>

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/20260908134739.96919-1-sj@kernel.org

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index c65d2cbeea6b5..f2f6f125c09b1 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -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)
> -						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?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 6.1.y v2 1/2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes()
  2026-10-09 14:56     ` sashiko-bot
@ 2026-10-09 15:09       ` SJ Park
  0 siblings, 0 replies; 11+ messages in thread
From: SJ Park @ 2026-10-09 15:09 UTC (permalink / raw)
  To: sashiko-bot; +Cc: SJ Park, damon

On Fri, 09 Oct 2026 14:56:56 +0000 sashiko-bot@kernel.org wrote:

> 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 <sj@kernel.org>
> 
> 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/20260227170623.95384-3-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 7afbc20833ea4..c65d2cbeea6b5 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -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.

Good catch.  I will fix this in the next spin.

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

This is an issue that exist in the mainline, too.  I will separately work on
it.

> 
> >  			damon_do_apply_schemes(c, t, r);
> >  	}
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261009144808.18317-1-sj@kernel.org?part=1


Thanks,
SJ

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 6.1.y v2 2/2] mm/damon/core: fix unconditionally skip last region
  2026-10-09 15:04     ` sashiko-bot
@ 2026-10-09 15:11       ` SJ Park
  0 siblings, 0 replies; 11+ messages in thread
From: SJ Park @ 2026-10-09 15:11 UTC (permalink / raw)
  To: sashiko-bot; +Cc: SJ Park, damon

On Fri, 09 Oct 2026 15:04:56 +0000 sashiko-bot@kernel.org wrote:

> 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 <aethernet65535@gmail.com>
> 
> 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/20260908134739.96919-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index c65d2cbeea6b5..f2f6f125c09b1 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -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)
> > -						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?

Good finding.  Patch 1 should avoided this issue by removing the safe iterator
workaround, but I mistakenly didn't remove the workaround.  I will fix it in
the next spin.

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


Thanks,
SJ

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-10-09 15:11 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <2026092949-surpass-thimble-02a8@gregkh>
2026-09-30  9:48 ` [PATCH 6.1.y] mm/damon/core: fix unconditionally skip last region SJ Park
2026-09-30 10:05   ` sashiko-bot
2026-09-30 10:23     ` SJ Park
2026-10-02 14:19   ` Sasha Levin
2026-10-09 14:48 ` [PATCH 6.1.y v2 0/2] mm/damon/core: fix last region handling of damos_skip_charged_regions() SJ Park
2026-10-09 14:48   ` [PATCH 6.1.y v2 1/2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes() SJ Park
2026-10-09 14:56     ` sashiko-bot
2026-10-09 15:09       ` SJ Park
2026-10-09 14:48   ` [PATCH 6.1.y v2 2/2] mm/damon/core: fix unconditionally skip last region SJ Park
2026-10-09 15:04     ` sashiko-bot
2026-10-09 15:11       ` SJ Park

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).