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

["Lorenzo Stoakes (Oracle)" <ljs@kernel.org>]

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