Re: [PATCH v2 4/4] mm: memcontrol: fix unexpected massive positive number in memcg_state_val_in_pages()

Lorenzo Stoakes (Oracle)
2026-03-26T09:16:54Z

On Wed, Mar 25, 2026 at 10:13:25PM +0800, Qi Zheng wrote:
> From: Qi Zheng <zhengqi.arch@bytedance.com>
>
> 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@kernel.org>
> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>

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@kernel.org>

> ---
> 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.

> +
> + return val < 0 ? -res : res;
> }
>
> #ifdef CONFIG_MEMCG_V1
> --
> 2.20.1
>

Cheers, Lorenzo