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

[Harry Yoo (Oracle)]

On Wed, Mar 25, 2026 at 10:43:58AM +0900, Harry Yoo (Oracle) wrote:
> On Tue, Mar 24, 2026 at 07:31:28PM +0800, Qi Zheng wrote:
> > @@ -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?

Oops, I was about to say... "Oh, doesn't patch 4/4 in v2 need to have
Fixes: 7bd5bc3ce963 ("mm: memcg: normalize the value passed into memcg_rstat_updated()")
???"

but then I realized that I made a silly mistake here.

> val = val * unit = -16384 (-16KiB)
> val * unit / PAGE_SIZE = 0xFFFFFFFFFFFFC000 / PAGE_SIZE = 0x3FFFFFFFFFFFFF

Err, I should have divided it by 0x1000, not 0x4096.

val * unit / PAGE_SIZE = 0xFFFFFFFFFFFFC000 / 0x1000 = 0xFFFFFFFFFFFFC
max(val * unit / PAGE_SIZE, 1UL) = 0xFFFFFFFFFFFFC
(int)0xFFFFFFFFFFFFC = -4.

> max(val * unit / PAGE_SIZE, 1UL) = 0x3FFFFFFFFFFFFF
> (int)0x3FFFFFFFFFFFFF = 0xFFFFFFFF = (-1)
> 
> That's incorrect. It should have been -4?

So that was correct.

The existing logic produces an accurate number (intended or not) as it
is right-shifted to only PAGE_SHIFT bits and truncated to int.

The existing logic is fine, it'll only be a problem when it's not
truncated to int.

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

I was wrong, not sashiko!

> Sounds like it's been subtly broken even before this patch and nobody
> noticed.

No, it's not.

-- 
Cheers,
Harry / Hyeonggon