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

[sashiko-bot@kernel.org]

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 <sj@kernel.org>
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/20260328133216.9697-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
> @@ -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?

[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?

> +	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?

[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?

[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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260801173554.94710-1-sj@kernel.org?part=7