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

[Liew Rui Yan <aethernet65535@gmail.com>]

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