[PATCH v2] mm/damon/core: fix unconditionally skip last region

[PATCH v2] mm/damon/core: fix unconditionally skip last region

[Liew Rui Yan]

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 resets the charge_{target,addr}_from when last region
is reached, only skips when it applied or cannot split.

Fixes: 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
Cc: <stable@vger.kernel.org> # v5.16.x
Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
---

Changes from v1:
- Simplify example in commit message.
- Add clarification regarding the severity of the bug to avoid causing
  users unnecessary afraid.
- Modify the patch code, each time the last region is reached, only
  charge_{target,addr}_from is reset, but the return value depends on
  the situation.
- Modify patch's title from 'mm/damon' to 'mm/damon/core', since it only
  changes core.c.
- v1: https://lore.kernel.org/damon/20260828084737.290024-1-aethernet65535@gmail.com

Changes from RFC v1:
- Minimal fix, only fixes the issue where the last-region is skipped.
- Add an example to the commit message to demonstrate that this error
  occurs very rarely.
- RFC v1: https://lore.kernel.org/damon/20260825124616.5129-1-aethernet65535@gmail.com

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

diff --git a/mm/damon/core.c b/mm/damon/core.c
index 644daf5a1656..fb76b530bbc6 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2342,36 +2342,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,
-- 
2.55.0

Re: [PATCH v2] mm/damon/core: fix unconditionally skip last region

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] A preexisting bug causes DAMOS scheme application to permanently stall for all subsequent targets if `quota->charge_target_from` points to a target that becomes invalid or loses all its regions.
--

commit 630d73d512e66245612cbdfe5c49647aa6ed2874
Author: Liew Rui Yan <aethernet65535@gmail.com>

mm/damon/core: fix unconditionally skip last region

This patch fixes an issue where the last region of a tracked target could
be unconditionally skipped and reset without being processed when a scheme
quota is exhausted. By modifying the reset logic, it ensures the last region
is appropriately handled rather than always skipped.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 644daf5a16560..fb76b530bbc68 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2342,36 +2342,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;

[Severity: High]
This isn't a bug introduced by this patch, but does this logic cause all
subsequent targets to permanently stall if the target currently stored in
quota->charge_target_from becomes invalid?

If the monitored process dies before the next window, kdamond_apply_schemes()
appears to bypass it without clearing the saved target:

mm/damon/core.c:kdamond_apply_schemes() {
    ...
		if (c->ops.target_valid && c->ops.target_valid(t) == false)
			continue;
		damos_apply_target(c, t, max_region_sz);
    ...
}

Because the target is skipped entirely, we never call damos_apply_target()
and never reach the clearing mechanism inside damos_skip_charged_region().

When this happens, wouldn't all other valid targets continue to evaluate
(t != quota->charge_target_from) as true here, permanently skipping scheme
applications?

It appears a local user could reliably trigger this by setting a restrictive
quota on a memory-intensive process they own, waiting for the quota to
exhaust, and then terminating the process.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830063159.6347-1-aethernet65535@gmail.com?part=1

Re: [PATCH v2] mm/damon/core: fix unconditionally skip last region

[Liew Rui Yan]

On Sun, 30 Aug 2026 06:47:07 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] A preexisting bug causes DAMOS scheme application to permanently stall for all subsequent targets if `quota->charge_target_from` points to a target that becomes invalid or loses all its regions.
> --
> 
> commit 630d73d512e66245612cbdfe5c49647aa6ed2874
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> 
> mm/damon/core: fix unconditionally skip last region
> 
> This patch fixes an issue where the last region of a tracked target could
> be unconditionally skipped and reset without being processed when a scheme
> quota is exhausted. By modifying the reset logic, it ensures the last region
> is appropriately handled rather than always skipped.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 644daf5a16560..fb76b530bbc68 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2342,36 +2342,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;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this logic cause all
> subsequent targets to permanently stall if the target currently stored in
> quota->charge_target_from becomes invalid?
> 
> If the monitored process dies before the next window, kdamond_apply_schemes()
> appears to bypass it without clearing the saved target:
> 
> mm/damon/core.c:kdamond_apply_schemes() {
>     ...
> 		if (c->ops.target_valid && c->ops.target_valid(t) == false)
> 			continue;
> 		damos_apply_target(c, t, max_region_sz);
>     ...
> }
> 
> Because the target is skipped entirely, we never call damos_apply_target()
> and never reach the clearing mechanism inside damos_skip_charged_region().
> 
> When this happens, wouldn't all other valid targets continue to evaluate
> (t != quota->charge_target_from) as true here, permanently skipping scheme
> applications?
> 
> It appears a local user could reliably trigger this by setting a restrictive
> quota on a memory-intensive process they own, waiting for the quota to
> exhaust, and then terminating the process.

Thank you for pointing this out!  Yes, this can happens.

I just wrote a simple fix, this fix introduces damon_target->'bool
has_charged', which was added to prevent entering the
damon_target_remove_ref() (new function introduces by this fix) loop
every time an invalid target is encountered.  However, adding this field
may be unnecessary, or it might better to use something like 'unsigned
int charged_ref_count' instead.

'''
diff --git a/include/linux/damon.h b/include/linux/damon.h
index 0c8b7ddef9ab..19d46d792858 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -93,6 +93,7 @@ struct damon_region {
 struct damon_target {
 	struct pid *pid;
 	bool obsolete;
+	bool has_charged;
 /* private: */
 	/* Number of monitoring target regions of this target. */
 	unsigned int nr_regions;
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 644daf5a1656..6c14b2d4e9cd 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2630,6 +2630,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
 		if (damos_quota_is_full(quota, c->min_region_sz)) {
 			quota->charge_target_from = t;
 			quota->charge_addr_from = r->ar.end;
+			t->has_charged = true;
 		}
 	}
 	if (s->action != DAMOS_STAT)
@@ -3216,6 +3217,26 @@ static void damos_trace_stat(struct damon_ctx *c, struct damos *s)
 	trace_call__damos_stat_after_apply_interval(cidx, sidx, &s->stat);
 }
 
+static void damon_target_remove_ref(struct damon_ctx *c,
+					 struct damon_target *t)
+{
+	struct damos *s;
+
+	damon_for_each_scheme(s, c) {
+		struct damos_quota *quota = &s->quota;
+
+		if (!quota->charge_target_from)
+			continue;
+
+		if (quota->charge_target_from == t) {
+			quota->charge_target_from = NULL;
+			quota->charge_addr_from = 0;
+		}
+	}
+
+	t->has_charged = false;
+}
+
 static void kdamond_apply_schemes(struct damon_ctx *c)
 {
 	struct damon_target *t;
@@ -3241,8 +3262,11 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
 	max_region_sz = damon_region_sz_limit(c);
 	mutex_lock(&c->walk_control_lock);
 	damon_for_each_target(t, c) {
-		if (c->ops.target_valid && c->ops.target_valid(t) == false)
+		if (c->ops.target_valid && c->ops.target_valid(t) == false) {
+			if (t->has_charged)
+				damon_target_remove_ref(c, t);
 			continue;
+		}
 		damos_apply_target(c, t, max_region_sz);
 	}
'''

SJ, when should this bug be fixed?  Along with this patch, or before
this patch?

> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260830063159.6347-1-aethernet65535@gmail.com?part=1

Best regards,
Rui Yan

Re: [PATCH v2] mm/damon/core: fix unconditionally skip last region

[SJ Park]

On Sun, 30 Aug 2026 14:31:50 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> 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 resets the charge_{target,addr}_from when last region
> is reached, only skips when it applied or cannot split.

Looks good to me!

> 
> Fixes: 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
> Cc: <stable@vger.kernel.org> # v5.16.x
> Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>

Reviewed-by: SJ Park <sj@kernel.org>


Thanks,
SJ

[...]

Re: [PATCH v2] mm/damon/core: fix unconditionally skip last region

[SJ Park]

On Sun, 30 Aug 2026 11:14:52 -0700 SJ Park <sj@kernel.org> wrote:

> On Sun, 30 Aug 2026 14:31:50 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> 
> > 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 resets the charge_{target,addr}_from when last region
> > is reached, only skips when it applied or cannot split.
> 
> Looks good to me!
> 
> > 
> > Fixes: 50585192bc2e ("mm/damon/schemes: skip already charged targets and regions")
> > Cc: <stable@vger.kernel.org> # v5.16.x
> > Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com>
> 
> Reviewed-by: SJ Park <sj@kernel.org>

This patch is applied to damon/next [1] tree.  If this patch is not added to
mm.git in short term (~1 week?), I will ask mm.git maintainer (Andrew Morton)
to pick this.  So, no action from your side is needed for now.  If it seems I
also forgot doing that or you cannot wait for my action, please feel free to
directly ask that to Andrew.

[1] https://origin.kernel.org/doc/html/latest/mm/damon/maintainer-profile.html#scm-trees


Thanks,
SJ

[...]

Re: [PATCH v2] mm/damon/core: fix unconditionally skip last region

[SJ Park]

On Sun, 30 Aug 2026 16:29:46 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> On Sun, 30 Aug 2026 06:47:07 +0000 sashiko-bot@kernel.org wrote:
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > 
> > Pre-existing issues:
> > - [High] A preexisting bug causes DAMOS scheme application to permanently stall for all subsequent targets if `quota->charge_target_from` points to a target that becomes invalid or loses all its regions.
> > --
> > 
> > commit 630d73d512e66245612cbdfe5c49647aa6ed2874
> > Author: Liew Rui Yan <aethernet65535@gmail.com>
> > 
> > mm/damon/core: fix unconditionally skip last region
> > 
> > This patch fixes an issue where the last region of a tracked target could
> > be unconditionally skipped and reset without being processed when a scheme
> > quota is exhausted. By modifying the reset logic, it ensures the last region
> > is appropriately handled rather than always skipped.
> > 
> > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > index 644daf5a16560..fb76b530bbc68 100644
> > > --- a/mm/damon/core.c
> > > +++ b/mm/damon/core.c
> > > @@ -2342,36 +2342,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;
> > 
> > [Severity: High]
> > This isn't a bug introduced by this patch, but does this logic cause all
> > subsequent targets to permanently stall if the target currently stored in
> > quota->charge_target_from becomes invalid?
> > 
> > If the monitored process dies before the next window, kdamond_apply_schemes()
> > appears to bypass it without clearing the saved target:
> > 
> > mm/damon/core.c:kdamond_apply_schemes() {
> >     ...
> > 		if (c->ops.target_valid && c->ops.target_valid(t) == false)
> > 			continue;
> > 		damos_apply_target(c, t, max_region_sz);
> >     ...
> > }
> > 
> > Because the target is skipped entirely, we never call damos_apply_target()
> > and never reach the clearing mechanism inside damos_skip_charged_region().
> > 
> > When this happens, wouldn't all other valid targets continue to evaluate
> > (t != quota->charge_target_from) as true here, permanently skipping scheme
> > applications?
> > 
> > It appears a local user could reliably trigger this by setting a restrictive
> > quota on a memory-intensive process they own, waiting for the quota to
> > exhaust, and then terminating the process.
> 
> Thank you for pointing this out!  Yes, this can happens.
> 
> I just wrote a simple fix, this fix introduces damon_target->'bool
> has_charged', which was added to prevent entering the
> damon_target_remove_ref() (new function introduces by this fix) loop
> every time an invalid target is encountered.  However, adding this field
> may be unnecessary, or it might better to use something like 'unsigned
> int charged_ref_count' instead.
> 
> '''
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 0c8b7ddef9ab..19d46d792858 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -93,6 +93,7 @@ struct damon_region {
>  struct damon_target {
>  	struct pid *pid;
>  	bool obsolete;
> +	bool has_charged;

I'd prefer not introducing new field.

>  /* private: */
>  	/* Number of monitoring target regions of this target. */
>  	unsigned int nr_regions;
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 644daf5a1656..6c14b2d4e9cd 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2630,6 +2630,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
>  		if (damos_quota_is_full(quota, c->min_region_sz)) {
>  			quota->charge_target_from = t;
>  			quota->charge_addr_from = r->ar.end;
> +			t->has_charged = true;
>  		}
>  	}
>  	if (s->action != DAMOS_STAT)
> @@ -3216,6 +3217,26 @@ static void damos_trace_stat(struct damon_ctx *c, struct damos *s)
>  	trace_call__damos_stat_after_apply_interval(cidx, sidx, &s->stat);
>  }
>  
> +static void damon_target_remove_ref(struct damon_ctx *c,
> +					 struct damon_target *t)
> +{
> +	struct damos *s;
> +
> +	damon_for_each_scheme(s, c) {
> +		struct damos_quota *quota = &s->quota;
> +
> +		if (!quota->charge_target_from)
> +			continue;
> +
> +		if (quota->charge_target_from == t) {
> +			quota->charge_target_from = NULL;
> +			quota->charge_addr_from = 0;
> +		}
> +	}
> +
> +	t->has_charged = false;
> +}
> +
>  static void kdamond_apply_schemes(struct damon_ctx *c)
>  {
>  	struct damon_target *t;
> @@ -3241,8 +3262,11 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
>  	max_region_sz = damon_region_sz_limit(c);
>  	mutex_lock(&c->walk_control_lock);
>  	damon_for_each_target(t, c) {
> -		if (c->ops.target_valid && c->ops.target_valid(t) == false)
> +		if (c->ops.target_valid && c->ops.target_valid(t) == false) {
> +			if (t->has_charged)
> +				damon_target_remove_ref(c, t);
>  			continue;
> +		}
>  		damos_apply_target(c, t, max_region_sz);
>  	}

I think damos_adjust_quota() is a better place to do this.  For example,

'''
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -3530,6 +3530,22 @@ static void damos_trace_esz(struct damon_ctx *c, struct damos *s,
        trace_damos_esz(cidx, sidx, quota->esz);
 }

+static void damos_reset_invalid_charge_target_from(struct damos_quota *quota,
+               struct damon_ctx *c)
+{
+       struct damon_target *t;
+
+       t = quota->charge_target_from;
+       if (!t)
+               return;
+       if (!c->ops.target_valid)
+               return;
+       if (c->ops.target_valid(t))
+               return;
+       quota->charge_target_from = NULL;
+       quota->charge_addr_from = 0;
+}
+
 static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
 {
        struct damos_quota *quota = &s->quota;
@@ -3570,6 +3586,8 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
                        damos_trace_esz(c, s, quota);
        }

+       damos_reset_invalid_charge_target_from(quota, c);
+
        if (!c->ops.get_scheme_score)
                return;
'''

> '''
> 
> SJ, when should this bug be fixed?  Along with this patch, or before
> this patch?

Whenever the fix is ready.  If you don't mind, I will take this.


Thanks,
SJ

[...]

Re: [PATCH v2] mm/damon/core: fix unconditionally skip last region

[Liew Rui Yan]

On Sun, 30 Aug 2026 11:38:51 -0700 SJ Park <sj@kernel.org> wrote:

> On Sun, 30 Aug 2026 16:29:46 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> 
> > On Sun, 30 Aug 2026 06:47:07 +0000 sashiko-bot@kernel.org wrote:
> > 
> > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > 
> > > Pre-existing issues:
> > > - [High] A preexisting bug causes DAMOS scheme application to permanently stall for all subsequent targets if `quota->charge_target_from` points to a target that becomes invalid or loses all its regions.
> > > --
> > > 
> > > commit 630d73d512e66245612cbdfe5c49647aa6ed2874
> > > Author: Liew Rui Yan <aethernet65535@gmail.com>
> > > 
> > > mm/damon/core: fix unconditionally skip last region
> > > 
> > > This patch fixes an issue where the last region of a tracked target could
> > > be unconditionally skipped and reset without being processed when a scheme
> > > quota is exhausted. By modifying the reset logic, it ensures the last region
> > > is appropriately handled rather than always skipped.
> > > 
> > > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > > index 644daf5a16560..fb76b530bbc68 100644
> > > > --- a/mm/damon/core.c
> > > > +++ b/mm/damon/core.c
> > > > @@ -2342,36 +2342,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;
> > > 
> > > [Severity: High]
> > > This isn't a bug introduced by this patch, but does this logic cause all
> > > subsequent targets to permanently stall if the target currently stored in
> > > quota->charge_target_from becomes invalid?
> > > 
> > > If the monitored process dies before the next window, kdamond_apply_schemes()
> > > appears to bypass it without clearing the saved target:
> > > 
> > > mm/damon/core.c:kdamond_apply_schemes() {
> > >     ...
> > > 		if (c->ops.target_valid && c->ops.target_valid(t) == false)
> > > 			continue;
> > > 		damos_apply_target(c, t, max_region_sz);
> > >     ...
> > > }
> > > 
> > > Because the target is skipped entirely, we never call damos_apply_target()
> > > and never reach the clearing mechanism inside damos_skip_charged_region().
> > > 
> > > When this happens, wouldn't all other valid targets continue to evaluate
> > > (t != quota->charge_target_from) as true here, permanently skipping scheme
> > > applications?
> > > 
> > > It appears a local user could reliably trigger this by setting a restrictive
> > > quota on a memory-intensive process they own, waiting for the quota to
> > > exhaust, and then terminating the process.
> > 
> > Thank you for pointing this out!  Yes, this can happens.
> > 
> > I just wrote a simple fix, this fix introduces damon_target->'bool
> > has_charged', which was added to prevent entering the
> > damon_target_remove_ref() (new function introduces by this fix) loop
> > every time an invalid target is encountered.  However, adding this field
> > may be unnecessary, or it might better to use something like 'unsigned
> > int charged_ref_count' instead.
> > 
> > '''
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 0c8b7ddef9ab..19d46d792858 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -93,6 +93,7 @@ struct damon_region {
> >  struct damon_target {
> >  	struct pid *pid;
> >  	bool obsolete;
> > +	bool has_charged;
> 
> I'd prefer not introducing new field.
> 
> >  /* private: */
> >  	/* Number of monitoring target regions of this target. */
> >  	unsigned int nr_regions;
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 644daf5a1656..6c14b2d4e9cd 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2630,6 +2630,7 @@ static void damos_apply_scheme(struct damon_ctx *c, struct damon_target *t,
> >  		if (damos_quota_is_full(quota, c->min_region_sz)) {
> >  			quota->charge_target_from = t;
> >  			quota->charge_addr_from = r->ar.end;
> > +			t->has_charged = true;
> >  		}
> >  	}
> >  	if (s->action != DAMOS_STAT)
> > @@ -3216,6 +3217,26 @@ static void damos_trace_stat(struct damon_ctx *c, struct damos *s)
> >  	trace_call__damos_stat_after_apply_interval(cidx, sidx, &s->stat);
> >  }
> >  
> > +static void damon_target_remove_ref(struct damon_ctx *c,
> > +					 struct damon_target *t)
> > +{
> > +	struct damos *s;
> > +
> > +	damon_for_each_scheme(s, c) {
> > +		struct damos_quota *quota = &s->quota;
> > +
> > +		if (!quota->charge_target_from)
> > +			continue;
> > +
> > +		if (quota->charge_target_from == t) {
> > +			quota->charge_target_from = NULL;
> > +			quota->charge_addr_from = 0;
> > +		}
> > +	}
> > +
> > +	t->has_charged = false;
> > +}
> > +
> >  static void kdamond_apply_schemes(struct damon_ctx *c)
> >  {
> >  	struct damon_target *t;
> > @@ -3241,8 +3262,11 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
> >  	max_region_sz = damon_region_sz_limit(c);
> >  	mutex_lock(&c->walk_control_lock);
> >  	damon_for_each_target(t, c) {
> > -		if (c->ops.target_valid && c->ops.target_valid(t) == false)
> > +		if (c->ops.target_valid && c->ops.target_valid(t) == false) {
> > +			if (t->has_charged)
> > +				damon_target_remove_ref(c, t);
> >  			continue;
> > +		}
> >  		damos_apply_target(c, t, max_region_sz);
> >  	}
> 
> I think damos_adjust_quota() is a better place to do this.  For example,
> 
> '''
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3530,6 +3530,22 @@ static void damos_trace_esz(struct damon_ctx *c, struct damos *s,
>         trace_damos_esz(cidx, sidx, quota->esz);
>  }
> 
> +static void damos_reset_invalid_charge_target_from(struct damos_quota *quota,
> +               struct damon_ctx *c)
> +{
> +       struct damon_target *t;
> +
> +       t = quota->charge_target_from;
> +       if (!t)
> +               return;
> +       if (!c->ops.target_valid)
> +               return;
> +       if (c->ops.target_valid(t))
> +               return;
> +       quota->charge_target_from = NULL;
> +       quota->charge_addr_from = 0;
> +}
> +
>  static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
>  {
>         struct damos_quota *quota = &s->quota;
> @@ -3570,6 +3586,8 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
>                         damos_trace_esz(c, s, quota);
>         }
> 
> +       damos_reset_invalid_charge_target_from(quota, c);
> +
>         if (!c->ops.get_scheme_score)
>                 return;
> '''
> 
> > '''
> > 
> > SJ, when should this bug be fixed?  Along with this patch, or before
> > this patch?
> 
> Whenever the fix is ready.  If you don't mind, I will take this.

I do not mind, thank you :>

Best regards,
Rui Yan

Re: [PATCH v2] mm/damon/core: fix unconditionally skip last region

[SJ Park]

On Mon, 31 Aug 2026 08:57:56 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:

> On Sun, 30 Aug 2026 11:38:51 -0700 SJ Park <sj@kernel.org> wrote:
> 
> > On Sun, 30 Aug 2026 16:29:46 +0800 Liew Rui Yan <aethernet65535@gmail.com> wrote:
> > 
> > > On Sun, 30 Aug 2026 06:47:07 +0000 sashiko-bot@kernel.org wrote:
> > > 
> > > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > > 
> > > > Pre-existing issues:
> > > > - [High] A preexisting bug causes DAMOS scheme application to permanently stall for all subsequent targets if `quota->charge_target_from` points to a target that becomes invalid or loses all its regions.
[...]
> > > SJ, when should this bug be fixed?  Along with this patch, or before
> > > this patch?
> > 
> > Whenever the fix is ready.  If you don't mind, I will take this.
> 
> I do not mind, thank you :>

Thank you for confirming, Liew!


Thanks,
SJ

[...]