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

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

On Fri, 28 Aug 2026 09:04:09 +0000 sashiko-bot@kernel.org wrote:

> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 644daf5a16560..21dc6b086c42f 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2347,14 +2347,15 @@ static bool damos_skip_charged_region(struct damon_target *t,
> >  	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)
> > +				r->ar.end <= quota->charge_addr_from) {
> > +			if (r->ar.end == quota->charge_addr_from ||
> > +					damon_is_last_region(r, t)) {
> > +				quota->charge_target_from = NULL;
> > +				quota->charge_addr_from = 0;
> > +			}
> >  			return true;
> > +		}
> >  
> >  		if (quota->charge_addr_from && r->ar.start <
> >  				quota->charge_addr_from) {
> 
> [Severity: High]
> If a target's last region straddles quota->charge_addr_from (meaning
> r->ar.start < quota->charge_addr_from but r->ar.end > quota->charge_addr_from),
> does nesting the damon_is_last_region() check strictly inside the
> r->ar.end <= quota->charge_addr_from condition cause it to bypass the reset
> logic entirely?
> 
> Because the unconditional r == damon_last_region(t) reset at the start of
> the function was removed, the code now proceeds to the next block where it
> attempts to skip the prefix:
> 
>     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;
>             sz_to_skip = min_region_sz;
>         }
>         damon_split_region_at(t, r, sz_to_skip);
>         return true;
>     }
> 
> If the prefix length is smaller than min_region_sz and
> damon_sz_region(r) <= min_region_sz, or if the subsequent
> damon_split_region_at() fails due to -ENOMEM, the function returns true early
> without splitting and without clearing quota->charge_target_from.
> 
> Does this permanently leak quota->charge_target_from?
> 
> If the function returns true early without clearing quota->charge_target_from,
> the region loop in damos_apply_target() would naturally terminate since it is
> the last region, leaving the cursor permanently pointing to the current target.
> 
> In subsequent time windows, could this leaked target repeatedly trigger the
> same early return, causing all other targets to be skipped permanently
> because t != quota->charge_target_from?

Thank you for pointing out this issue!

I just wrote a v2:

'''
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 21dc6b086c42..02a4b2edcc4b 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2348,14 +2348,8 @@ static bool damos_skip_charged_region(struct damon_target *t,
                if (t != quota->charge_target_from)
                        return true;
                if (quota->charge_addr_from &&
-                               r->ar.end <= quota->charge_addr_from) {
-                       if (r->ar.end == quota->charge_addr_from ||
-                                       damon_is_last_region(r, t)) {
-                               quota->charge_target_from = NULL;
-                               quota->charge_addr_from = 0;
-                       }
-                       return true;
-               }
+                               r->ar.end <= quota->charge_addr_from)
+                       goto may_reset;
 
                if (quota->charge_addr_from && r->ar.start <
                                quota->charge_addr_from) {
@@ -2363,16 +2357,24 @@ static bool damos_skip_charged_region(struct damon_target *t,
                                        r->ar.start, min_region_sz);
                        if (!sz_to_skip) {
                                if (damon_sz_region(r) <= min_region_sz)
-                                       return true;
+                                       goto may_reset;
                                sz_to_skip = min_region_sz;
                        }
-                       damon_split_region_at(t, r, sz_to_skip);
+                       if (damon_split_region_at(t, r, sz_to_skip))
+                               goto may_reset;
                        return true;
                }
                quota->charge_target_from = NULL;
                quota->charge_addr_from = 0;
        }
        return false;
+
+may_reset:
+       if (damon_is_last_region(r, t)) {
+               quota->charge_target_from = NULL;
+               quota->charge_addr_from = 0;
+       }
+       return true;
 }
'''

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

Best regards,
Rui Yan