Re: [PATCH 2/3] mm: memcontrol: correct the parameter type of __mod_memcg{_lruvec}_state()

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

On 3/25/26 9:43 AM, Harry Yoo (Oracle) wrote:
> On Tue, Mar 24, 2026 at 07:31:28PM +0800, Qi Zheng wrote:
>> From: Qi Zheng <zhengqi.arch@bytedance.com>
>>
>> The __mod_memcg_state() and __mod_memcg_lruvec_state() were used to
>> reparent non-hierarchical stats, the values passed to them might exceed
>> the upper limit of the type int, so correct the val parameter type of them
>> to long.
>>
>> Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
>> ---
>>   include/trace/events/memcg.h | 10 +++++-----
>>   mm/memcontrol.c              |  8 ++++----
>>   2 files changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 7fb9cbc10dfbb..4a78550f6174e 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -527,7 +527,7 @@ unsigned long lruvec_page_state_local(struct lruvec *lruvec,
>>   
>>   #ifdef CONFIG_MEMCG_V1
>>   static void __mod_memcg_lruvec_state(struct mem_cgroup_per_node *pn,
>> -				     enum node_stat_item idx, int val);
>> +				     enum node_stat_item idx, long val);
>>   
>>   void reparent_memcg_lruvec_state_local(struct mem_cgroup *memcg,
>>   				       struct mem_cgroup *parent, int idx)
>> @@ -784,7 +784,7 @@ static int memcg_page_state_unit(int item);
>>    * Normalize the value passed into memcg_rstat_updated() to be in pages. Round
>>    * up non-zero sub-page updates to 1 page as zero page updates are ignored.
>>    */
>> -static int memcg_state_val_in_pages(int idx, int val)
>> +static long memcg_state_val_in_pages(int idx, long val)
>>   {
>>   	int unit = memcg_page_state_unit(idx);
> 
> Sashiko AI made an interesting argument [1] that this could lead to
> incorrectly returning a very large positive number. Let me verify that.
> 
> [1] https://sashiko.dev/#/patchset/cover.1774342371.git.zhengqi.arch%40bytedance.com
> 
> Sashiko wrote:
>> Does this change inadvertently break the handling of negative byte-sized
>> updates?
>> Looking at the rest of the function:
>> 	if (!val || unit == PAGE_SIZE)
>> 		return val;
>> 	else
>> 		return max(val * unit / PAGE_SIZE, 1UL);
> 
>> PAGE_SIZE is defined as an unsigned long.
> 
> Right, it's defined as 1UL << PAGE_SHIFT.
> 
>> When val is negative, such as during uncharging of byte-sized stats like
>> MEMCG_ZSWAP_B, the expression val * unit is a negative long.
> 
> Right.
> 
>> Dividing a signed long by an unsigned long causes the signed long to be
>> promoted to unsigned before division,
> 
> Right.
> 
>> resulting in a massive positive
>> number instead of a small negative one.
> 
> Let's look at an example (assuming unit is 1).
> 
> val = val * unit = -16384 (-16 KiB)
> val * unit / PAGE_SIZE = 0xFFFFFFFFFFFFC000 / PAGE_SIZE = 0x3FFFFFFFFFFFFF
> max(0x3FFFFFFFFFFFFF, 1UL) = 0x3FFFFFFFFFF
> 
> Yeah, that's a massive positive number.
> 
> Hmm but how did it work when it was int?
> 
> val = val * unit = -16384 (-16KiB)
> val * unit / PAGE_SIZE = 0xFFFFFFFFFFFFC000 / PAGE_SIZE = 0x3FFFFFFFFFFFFF
> max(val * unit / PAGE_SIZE, 1UL) = 0x3FFFFFFFFFFFFF
> (int)0x3FFFFFFFFFFFFF = 0xFFFFFFFF = (-1)
> 
> That's incorrect. It should have been -4?
> 
>> Before this change, the function returned an int, which implicitly truncated
>> the massive unsigned 64-bit result to a 32-bit int, accidentally yielding the
>> correct negative arithmetic value.
> 
> So... "accidentally yielding the correct negative arithemetic value"
> is wrong.
> 
> Sounds like it's been subtly broken even before this patch and nobody
> noticed.

Thank you for such a detailed analysis! And I think you are right.

The memcg_state_val_in_pages() is only to make @val to be in pages, so
perhaps we can avoid the above problem by taking the absolute value
first?

Thanks,
Qi

> 
>> By changing the return type to long, this implicit truncation is removed,
>> and the huge positive value is returned unaltered.
> 
> That's true.
> 
>> Could this corrupt tracepoint logs when passed to trace_mod_memcg_state?
> 
> I'm not sure if that's critical but yeah that's true.
> 
>> Also, would passing this huge positive value to memcg_rstat_updated instantly
>> exceed the charge batch threshold and trigger endless, expensive global
>> cgroup_rstat flushing, severely degrading system performance?
> 
> It would lead to more frequent flushes, at least.
>