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

Qi Zheng
2026-03-26T09:32:53Z

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

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@bytedance.com/

Thanks,
Qi

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