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

[Qi Zheng <qi.zheng@linux.dev>]

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