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

On Sat, 01 Aug 2026 19:11:30 +0000 [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> 
> New issues:
> - [Medium] Falsely reporting 100% memory utilization for an empty node due to a flawed division-by-zero fix.
> 
> Pre-existing issues:
> - [High] Potential integer underflow when evaluating `i.totalram - i.freeram` due to lockless vmstat reads.
> - [High] The patch fixes a potential divide-by-zero in `damos_get_node_mem_bp()`, but fails to apply the same fix to `damos_get_node_memcg_used_bp()`.
> - [High] Multiplication overflow in basis-point calculation on 32-bit architectures.
> - [High] Potential divide by zero in `damos_get_in_active_mem_bp()` if the system has no active or inactive LRU pages.
> --
> 
> --- Patch [7]: [RFC PATCH 7/9] mm/damon/core: handle extreme memory state in damon_get_node_mem_bp() ---
> commit 0e2bb1b3ade5000769644aa674efbcfec5b43152
> Author: SJ Park <[email protected]>
> Subject: 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/[email protected]
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2816,6 +2816,8 @@ static __kernel_ulong_t damos_get_node_mem_bp(
> >  	}
> >  
> >  	si_meminfo_node(&i, goal->nid);
> > +	if (!i.totalram)
> > +		i.totalram = 1;
> > +	if (goal->metric == DAMOS_QUOTA_NODE_MEM_USED_BP)
> > +		numerator = i.totalram - i.freeram;
> 
> [Severity: Medium]
> Does setting totalram to 1 artificially inflate the utilization for an empty
> node?
> 
> If si_meminfo_node() returns 0 for both totalram and freeram, numerator
> becomes 1 - 0 = 1. This would result in 100 percent used memory (10000 basis
> points), which contradicts the fallback logic that reports 0 percent for an
> empty node. Will this trigger DAMOS quota actions erroneously?

I think this is fine.  Just different perspectives, and no real problem in real
life.  We can later revisit for consistent interpretation of this extreme
situations.

> 
> [Severity: High]
> This is a pre-existing issue, but can this subtraction underflow?
> 
> Because si_meminfo_node() computes freeram via lockless zone stats, per-CPU
> stat lag can temporarily cause freeram to exceed totalram. If freeram >
> totalram, the unsigned long numerator will underflow to a massive value.
> 
> If we force totalram to 1 here and freeram is greater than 0, doesn't it
> guarantee this underflow will occur?

Good point, I will fix in the next version.

> 
> > +	else	/* DAMOS_QUOTA_NODE_MEM_FREE_BP */
> 
> [ ... ]
> 
> [Severity: High]
> This is a pre-existing issue, but does the same divide-by-zero happen in the
> adjacent damos_get_node_memcg_used_bp() function?
> 
> mm/damon/core.c:damos_get_node_memcg_used_bp() {
>         ...
>         si_meminfo_node(&i, goal->nid);
>         ...
>         return mult_frac(numerator, 10000, i.totalram);
> }
> 
> Since si_meminfo_node() can return 0 for totalram, wouldn't this also cause
> a divide-by-zero here if i.totalram isn't validated?

A later patch in this series will fix it.

> 
> [Severity: High]
> This is a pre-existing issue, but can the basis-point calculation overflow
> on 32-bit architectures?
> 
> mm/damon/core.c:damos_get_node_mem_bp() {
>         ...
>         return mult_frac(numerator, 10000, i.totalram);
> }
> 
> Since mult_frac(numerator, 10000, denom) computes the remainder times 10000,
> if the remainder exceeds roughly 429,496, won't this overflow a 32-bit
> unsigned long and produce an incorrect result?

Known low priority issue.

> 
> [Severity: High]
> This is a pre-existing issue, but is there a potential divide-by-zero here
> if the system has no active or inactive LRU pages?
> 
> mm/damon/core.c:damos_get_in_active_mem_bp() {
>         ...
>         total = active + inactive;
>         if (active_ratio)
>                 return mult_frac(active, 10000, total);
>         return mult_frac(inactive, 10000, total);
> }
> 
> If all memory is mlocked or purely kernel-allocated, both active and
> inactive could be 0, making total 0. Will mult_frac() panic when dividing
> by total?

A later patch in this series will fix it.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7


Thanks,
SJ