Re: [PATCH v3 2/3] mm: memcontrol: change val type to long in __mod_memcg_{lruvec_}state()

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

On 3/30/26 9:25 AM, Harry Yoo (Oracle) wrote:
> On Fri, Mar 27, 2026 at 06:16:29PM +0800, Qi Zheng wrote:
>> From: Qi Zheng <zhengqi.arch@bytedance.com>
>>
>> The __mod_memcg_state() and __mod_memcg_lruvec_state() functions are also
>> used to reparent non-hierarchical stats. In this scenario, the values
>> passed to them are accumulated statistics that might be extremely large
>> and exceed the upper limit of a 32-bit integer.
>>
>> Change the val parameter type from int to long in these functions and
>> their corresponding tracepoints (memcg_rstat_stats) to prevent potential
>> overflow issues.
>>
>> After that, 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>
>> Reviewed-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org>
>> ---
> 
> Looks good to me,
> Reviewed-by: Harry Yoo (Oracle) <harry@kernel.org>

Thanks!

> 
>> @@ -831,7 +837,7 @@ static inline void get_non_dying_memcg_end(void)
>>   #endif
>>   
>>   static void __mod_memcg_state(struct mem_cgroup *memcg,
>> -			      enum memcg_stat_item idx, int val)
>> +			      enum memcg_stat_item idx, long val)
>>   {
>>   	int i = memcg_stats_index(idx);
>>   	int cpu;
>> @@ -896,7 +902,7 @@ void reparent_memcg_state_local(struct mem_cgroup *memcg,
>>   #endif
>>   
>>   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)
>>   {
>>   	struct mem_cgroup *memcg = pn->memcg;
>>   	int i = memcg_stats_index(idx);
> 
> Some of code paths that calls mod_memcg{,_lruvec}_state still passes
> int values (which is quite subtle to notice), but it should be fine

Right, it happens in too many places, for example, the callers of
mod_lruvec_state().

> as reparenting is not involved in the path and could be cleaned up later.

Agree.

>