damon.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/damon/core: reset invalid quota->charge_target_from
@ 2026-09-07 16:55 SJ Park
  2026-09-07 17:13 ` sashiko-bot
  2026-09-07 17:28 ` SJ Park
  0 siblings, 2 replies; 4+ messages in thread
From: SJ Park @ 2026-09-07 16:55 UTC (permalink / raw)
  To: Andrew Morton; +Cc: SJ Park, stable, Enze Li, damon, linux-kernel, linux-mm

DAMOS can suddenly stop working if a target process that quota just
fully charged in it is terminated.  Fix by catching and processing the
corner case.

When DAMOS quota is fully charged, the target and the region to continue
applying the action in the next round is saved in
damos_quota->charge_{target,addr}_from.  In the next round, DAMOS
iterates targets and regions from the beginning.  It skips applying the
action to the regions until it visits and skips the saved target/region.

Virtual address space targets become invalid if the process is
terminated.  Trying to apply the scheme to invalid target is just a
waste of time.  Hence commit 6e4930e33329 ("mm/damon/core: fix wasteful
CPU calls by skipping non-existent targets") made the logic to skip
invalid targets.  However, it does skip before the charged target/region
skipping/updating.

Let's suppose the user runs DAMOS for multiple virtual address spaces
with a quota.  The quota exceeded in the middle of a virtual address
space.  And the process of the address space is terminated.  Then the
charge_target_from points to the invalid target.  The pointer update
logic is skipped for the invalid target, so the charge_target_from is
never updated.  DAMOS action to every target/region is skipped.  From
the user's perspective, it would look like suddenly DAMOS has stopped
working.

No critical leak or crash can happen.  The user could reinstall the
scheme.  But this makes use of DAMOS under certain setups quite
unreliable.

Catch the corner case and reset the pointer in damos_adjust_quota(),
which is called for adjustment of the quota for every round.

This issue was discovered [1] by Sashiko.

[1] https://lore.kernel.org/20260830064708.40CA61F000E9@smtp.kernel.org

Changes from RFC v1
- RFC v1: https://lore.kernel.org/20260906183654.95634-1-sj@kernel.org
- Drop RFC tag.

Fixes: 6e4930e33329 ("mm/damon/core: fix wasteful CPU calls by skipping non-existent targets")
Cc: <stable@vger.kernel.org> # 7.0.x
Signed-off-by: SJ Park <sj@kernel.org>
---
 mm/damon/core.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/mm/damon/core.c b/mm/damon/core.c
index ce8c6f99106ed..2c58be9edbde4 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -3283,6 +3283,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;
@@ -3318,6 +3334,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;
 

base-commit: 6ce2570ad1f1f4195c8d8b1a460fec2fc276dd65
-- 
2.47.3

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

* Re: [PATCH] mm/damon/core: reset invalid quota->charge_target_from
  2026-09-07 16:55 [PATCH] mm/damon/core: reset invalid quota->charge_target_from SJ Park
@ 2026-09-07 17:13 ` sashiko-bot
  2026-09-07 17:27   ` SJ Park
  2026-09-07 17:28 ` SJ Park
  1 sibling, 1 reply; 4+ messages in thread
From: sashiko-bot @ 2026-09-07 17:13 UTC (permalink / raw)
  To: SJ Park; +Cc: damon

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

Pre-existing issues:
- [High] The newly added fix for invalid `charge_target_from` pointers is bypassed if a scheme's quota is dynamically disabled, leaving the scheme permanently broken.
--

commit f88287bb2670346a5987d086e0731c7ba6d0b19e
Author: SJ Park <sj@kernel.org>

    mm/damon/core: reset invalid quota->charge_target_from
    
    DAMOS can suddenly stop working if a target process that quota just
    fully charged in it is terminated.  Fix by catching and processing the
    corner case.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index ce8c6f99106ed..2c58be9edbde4 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -3318,6 +3334,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;
>  

[Severity: High]
This is a pre-existing issue, but does this bypass the newly added reset if a
scheme's quota is dynamically disabled?

If a user hits the corner case where charge_target_from becomes invalid, and
then dynamically disables the scheme's quota via sysfs (setting ms=0 and
sz=0), damos_adjust_quota() will return early before the reset is reached:

mm/damon/core.c:damos_adjust_quota() {
	if (!quota->ms && !quota->sz && list_empty(&quota->goals))
		return;
	...
}

Because damos_skip_charged_region() does not check if the quota is enabled
before evaluating charge_target_from, it will continue to skip all regions
forever:

mm/damon/core.c:damos_skip_charged_region() {
	...
		if (t != quota->charge_target_from)
			return true;
	...
}

Could the pointer reset be moved before the early return in
damos_adjust_quota(), or could the skipping logic be updated to handle
disabled quotas?

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

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

* Re: [PATCH] mm/damon/core: reset invalid quota->charge_target_from
  2026-09-07 17:13 ` sashiko-bot
@ 2026-09-07 17:27   ` SJ Park
  0 siblings, 0 replies; 4+ messages in thread
From: SJ Park @ 2026-09-07 17:27 UTC (permalink / raw)
  To: sashiko-bot; +Cc: SJ Park, damon

On Mon, 07 Sep 2026 17:13:46 +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] The newly added fix for invalid `charge_target_from` pointers is bypassed if a scheme's quota is dynamically disabled, leaving the scheme permanently broken.
> --
> 
> commit f88287bb2670346a5987d086e0731c7ba6d0b19e
> Author: SJ Park <sj@kernel.org>
> 
>     mm/damon/core: reset invalid quota->charge_target_from
>     
>     DAMOS can suddenly stop working if a target process that quota just
>     fully charged in it is terminated.  Fix by catching and processing the
>     corner case.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index ce8c6f99106ed..2c58be9edbde4 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -3318,6 +3334,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;
> >  
> 
> [Severity: High]
> This is a pre-existing issue, but does this bypass the newly added reset if a
> scheme's quota is dynamically disabled?
> 
> If a user hits the corner case where charge_target_from becomes invalid, and
> then dynamically disables the scheme's quota via sysfs (setting ms=0 and
> sz=0), damos_adjust_quota() will return early before the reset is reached:
> 
> mm/damon/core.c:damos_adjust_quota() {
> 	if (!quota->ms && !quota->sz && list_empty(&quota->goals))
> 		return;
> 	...
> }
> 
> Because damos_skip_charged_region() does not check if the quota is enabled
> before evaluating charge_target_from, it will continue to skip all regions
> forever:
> 
> mm/damon/core.c:damos_skip_charged_region() {
> 	...
> 		if (t != quota->charge_target_from)
> 			return true;
> 	...
> }
> 
> Could the pointer reset be moved before the early return in
> damos_adjust_quota(), or could the skipping logic be updated to handle
> disabled quotas?

Good catch.  I think we should unset the charge_{target,addr}_from in
damos_commit_quota().  I will do so in the next version.

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


Thanks,
SJ

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

* Re: [PATCH] mm/damon/core: reset invalid quota->charge_target_from
  2026-09-07 16:55 [PATCH] mm/damon/core: reset invalid quota->charge_target_from SJ Park
  2026-09-07 17:13 ` sashiko-bot
@ 2026-09-07 17:28 ` SJ Park
  1 sibling, 0 replies; 4+ messages in thread
From: SJ Park @ 2026-09-07 17:28 UTC (permalink / raw)
  To: SJ Park; +Cc: Andrew Morton, stable, Enze Li, damon, linux-kernel, linux-mm

On Mon,  7 Sep 2026 09:55:27 -0700 SJ Park <sj@kernel.org> wrote:

> DAMOS can suddenly stop working if a target process that quota just
> fully charged in it is terminated.  Fix by catching and processing the
> corner case.

Sashiko found this fix is not complete.  I will post a new version.


Thanks,
SJ

[...]

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

end of thread, other threads:[~2026-09-07 17:28 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-07 16:55 [PATCH] mm/damon/core: reset invalid quota->charge_target_from SJ Park
2026-09-07 17:13 ` sashiko-bot
2026-09-07 17:27   ` SJ Park
2026-09-07 17:28 ` 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).