Re: [RFC PATCH v1.1 7/9] mm/damon/core: handle extreme memory state in damon_get_node_mem_bp()

[SJ Park <sj@kernel.org>]

On Sun,  2 Aug 2026 10:49:50 -0700 SJ Park <sj@kernel.org> wrote:

> On Sun, 02 Aug 2026 16:35:44 +0000 sashiko-bot@kernel.org wrote:
> 
> > Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> > 
> > New issues:
> > - [High] Returning 0 for DAMOS_QUOTA_NODE_MEM_FREE_BP when freeram > totalram incorrectly forces maximum scheme aggressiveness instead of reporting 100% free memory.
> > - [Medium] The commit message contradicts the implementation, and the unconditional return of 10000 for zero totalram breaks metric semantics.
> > 
> > Pre-existing issues:
> > - [High] Unhandled integer underflow in MEM_USED_BP and MEMCG_FREE_BP calculations when counters drift.
> > - [High] The patch fixes a potential divide-by-zero in damos_get_node_mem_bp() but misses identical unvalidated divisions in related functions.
> > --
> > 
> > --- Patch [7]: [RFC PATCH v1.1 7/9] mm/damon/core: handle extreme memory state in damon_get_node_mem_bp() ---
> > commit 05d27d4d51952669ff23ec9285ccb83a273556ad
> > Author: SJ Park <sj@kernel.org>
> > 
> > mm/damon/core: handle extreme memory state in damon_get_node_mem_bp()
> > 
> > In an extreme and unlikely situation, si_meminfo_node() might let the
> > caller show zero total ram. That could cause a divide by zero in
> > damon_get_node_mem_bp(). Fix it by setting the totalram one byte in the
> > case.
> > 
> > Link: https://lore.kernel.org/20260328133216.9697-1-sj@kernel.org
> > 
> > > In an extreme and unlikely situation, si_meminfo_node() might let the
> > > caller show zero total ram.  That could cause a divide by zero in
> > > damon_get_node_mem_bp().  Fix it by setting the totalram one byte in the
> > > case.
> > 
> > [Severity: Medium]
> > The commit message states "Fix it by setting the totalram one byte in the
> > case."
> > 
> > However, the implementation actually bypasses the metric type check and returns
> > 10000. 
> > 
> > Does this unconditional return contradict the intent documented in the commit
> > message?
> 
> Good catch, I will update the commit message.
> 
> > 
> > > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > > index 3bdbf4fbf7147..e3f3ee75a3d33 100644
> > > --- a/mm/damon/core.c
> > > +++ b/mm/damon/core.c
> > > @@ -2816,10 +2816,16 @@ static __kernel_ulong_t damos_get_node_mem_bp(
> > >  	}
> > >  
> > >  	si_meminfo_node(&i, goal->nid);
> > > -	if (goal->metric = DAMOS_QUOTA_NODE_MEM_USED_BP)
> > > +	if (!i.totalram)
> > > +		return 10000;
> > 
> > [Severity: Medium]
> > If the metric is DAMOS_QUOTA_NODE_MEM_FREE_BP, shouldn't it report 0% free
> > rather than 100% free if the memory is absent (which is what totalram=1,
> > freeram=0 would yield)?
> > 
> > Does returning 10000 here break the semantic constraint that used + free = 100%?
> 
> I made mistakes at revisioning.  I will fix this.

And this semantic gap exist in the next patch, too.  I will fix it together.

> 
> > 
> > > +	if (goal->metric = DAMOS_QUOTA_NODE_MEM_USED_BP) {
> > >  		numerator = i.totalram - i.freeram;
> > 
> > [Severity: High]
> > This is a pre-existing issue, but if counter drift results in
> > freeram > totalram, does this subtraction silently underflow, producing a
> > massive unsigned value?
> > 
> > If this is passed to mult_frac(), could it generate a huge metric value,
> > leading the auto-tuner to mistakenly act as if the memory is massively
> > overused?
> > 
> > An identical unhandled underflow also appears to exist in
> > damos_get_node_memcg_used_bp() when evaluating DAMOS_QUOTA_NODE_MEMCG_FREE_BP
> > if used_pages exceeds totalram:
> > 
> > mm/damon/core.c:damos_get_node_memcg_used_bp() {
> >     ...
> >     numerator = i.totalram - used_pages;
> >     ...
> > }
> 
> The intention of this revision was handling this, but I made a complete
> mistake.  I will fix.
> 
> > 
> > > -	else	/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
> > > +	} else {
> > > +		/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
> > > +		if (i.totalram < i.freeram)
> > > +			return 0;
> > 
> > [Severity: High]
> > When counter drift causes freeram > totalram, the node effectively has
> > ~100% free memory. 
> > 
> > By returning 0 (0% free memory) in this scenario, doesn't the DAMON quota
> > auto-tuning feedback loop interpret this as severely under-achieving the free
> > memory goal, causing it to wastefully and drastically increase the reclamation
> > budget (e.g., pageout/demotion) on a node that is already completely free?
> > 
> > Should this return 10000 (100% free) instead?
> 
> That depends on the perspective.  I think 0% makes sense.
> 
> > 
> > >  		numerator = i.freeram;
> > > +	}
> > >  	return mult_frac(numerator, 10000, i.totalram);
> > >  }
> > 
> > [Severity: High]
> > This is a pre-existing issue, but while this patch addresses a potential
> > divide-by-zero when si_meminfo_node() reports totalram = 0, does the same
> > unvalidated division exist in related functions?
> > 
> > For example, damos_get_node_memcg_used_bp() divides by i.totalram without a
> > zero-check:
> > 
> > mm/damon/core.c:damos_get_node_memcg_used_bp() {
> >     ...
> >     si_meminfo_node(&i, goal->nid);
> >     ...
> >     return mult_frac(numerator, 10000, i.totalram);
> > }
> > 
> > Similarly, damos_get_in_active_mem_bp() divides by total (active + inactive)
> > via mult_frac() without checking if total is zero:
> > 
> > mm/damon/core.c:damos_get_in_active_mem_bp() {
> >     ...
> >     if (goal->metric = DAMOS_QUOTA_SOME_MEM_USED_BP)
> >         return mult_frac(active, 10000, total);
> >     return mult_frac(inactive, 10000, total);
> > }
> 
> Later patches in this series fix those.
> 
> > 
> > -- 
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260802162631.90304-1-sj@kernel.org?part=7
> > 
> 
> 
> Thanks,
> SJ
> 


Thanks,
SJ