Re: [PATCH 6.12.y v3 2/2] mm/damon/core: fix unconditionally skip last region

[SJ Park <sj@kernel.org>]

On Fri, 09 Oct 2026 13:06:32 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] mm/damon: dangling charge_target_from pointer permanently freezes schemes
> --
> 
> commit a9baf64370898b2dec04fa2f17e4061166d6d27a
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> 
> mm/damon/core: fix unconditionally skip last region
> 
> This commit modifies damos_skip_charged_region() to clear the quota's
> charge_target_from pointer only when the last region of a target is
> reached, rather than clearing it unconditionally during previous charge
> checks.
> 
> Link: https://lore.kernel.org/20260908134739.96919-1-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1323,36 +1323,39 @@ static bool damos_skip_charged_region(struct damon_target *t,
> >  		struct damon_region *r, struct damos *s)
> >  {
> >  	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;
> 
> [ ... ]
> 
> > +	}
> > +out:
> > +	if (r = damon_last_region(t)) {
> >  		quota->charge_target_from = NULL;
> >  		quota->charge_addr_from = 0;
> >  	}
> 
> [Severity: High]
> If the target tracked by quota->charge_target_from is dynamically
> destroyed via sysfs, or if it loses all of its regions, will this
> clearing logic ever be reached?

Good finding.  But this is pre-existing issue that also exist in the mainline
tree.  So not a blocker of this patch in my opinion.  I will separately working
on this.

> 
> If the saved target is removed or has zero regions, the region loop will
> be empty, meaning r = damon_last_region(t) is never evaluated for
> that target.
> 
> Since charge_target_from is never cleared in these scenarios, wouldn't
> this cause the condition earlier in this function:
> 
>         if (t != quota->charge_target_from)
>                 return true;
> 
> to remain true indefinitely for all other valid targets, permanently
> freezing the scheme? Additionally, if the target is freed, does this
> leave a dangling pointer comparison for future checks?
> 
> > -	return false;
> > +	return skip;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261009123651.6585-1-sj@kernel.org?part=2
> 


Thanks,
SJ