Re: [PATCH v2 4/4] mm: memcontrol: fix unexpected massive positive number in memcg_state_val_in_pages()
From: Qi Zheng
Date: Thu Mar 26 2026 - 05:42:48 EST
On 3/26/26 5:16 PM, Lorenzo Stoakes (Oracle) wrote:
On Wed, Mar 25, 2026 at 10:13:25PM +0800, Qi Zheng wrote:
From: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
In memcg_state_val_in_pages(), if the passed val is negative, the
expression val * unit / PAGE_SIZE could be implicitly converted to a
massive positive number when compared with 1UL in the max() macro.
This leads to returning an incorrect massive positive value.
Fix this by using abs(val) to calculate the magnitude first, and then
restoring the sign of the value before returning the result. Additionally,
use mult_frac() to prevent potential overflow during the multiplication of
val and unit.
Reported-by: Harry Yoo (Oracle) <harry@xxxxxxxxxx>
Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
The logic is correct, but I think this needs rework for better
understanding, and obviously this should be squashed into 2/4 as per
Andrew.
With the below change applied:
Reviewed-by: Lorenzo Stoakes (Oracle) <ljs@xxxxxxxxxx>
---
mm/memcontrol.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 04076a139dbe3..0c249255ebefb 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -787,11 +787,14 @@ static int memcg_page_state_unit(int item);
static long memcg_state_val_in_pages(int idx, long val)
{
int unit = memcg_page_state_unit(idx);
+ long res;
if (!val || unit == PAGE_SIZE)
return val;
- else
- return max(val * unit / PAGE_SIZE, 1UL);
Hm this was already fairly horrid, because we're comparing an unsigned long
value of 1 vs. a ULONG_MAX - abs(val) so this was intended to make 0 -> 1UL
but not what you'd mathematically think this was which was to make negative
values (logically < 1) -> 1.
Of course before it was just broken and would promote (val * unit /
PAGE_SIZE) to unsigned long first (thus massive number) and return that :)
+
+ res = max(mult_frac(abs(val), unit, PAGE_SIZE), 1UL);
This is way too compressed into one line and retains the confusing
behaviour.
Could we split this out and explain what we're doing (sign-extension,
integer promotion and all of this stuff is confusing - so let's just accept
that and spell it out):
/* Get the absolute value of (val * unit / PAGE_SIZE). */
res = mult_frac(abs(val), unit, PAGE_SIZE);
/* Round up zero values. */
res = res ?: 1;
/* Retain sign. */
return val < 0 ? -res : res;
This is functionally identical, but a lot more readable, I think.
Make sense, I will update to v3.
If Andrew needs me to merge this patchset into "[PATCH v6 00/33] Eliminate Dying Memory Cgroup" [1], then I will develop and send v7.
[1]. https://lore.kernel.org/all/cover.1772711148.git.zhengqi.arch@xxxxxxxxxxxxx/
Thanks,
Qi
+
+ return val < 0 ? -res : res;
}
#ifdef CONFIG_MEMCG_V1
--
2.20.1
Cheers, Lorenzo