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

[SJ Park <sj@kernel.org>]

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

[...]