memcg: fix slab accounting in refill_obj_stock() trylock path

[PATCH] memcg: fix slab accounting in refill_obj_stock() trylock path

Posted by Hao Li 1 month, 1 week ago

In the trylock path of refill_obj_stock(), mod_objcg_mlstate() should
use the real alloc/free bytes (i.e., nr_acct) for accounting, rather
than nr_bytes.

Fixes: 200577f69f29 ("memcg: objcg stock trylock without irq disabling")
Cc: stable@vger.kernel.org
Signed-off-by: Hao Li <hao.li@linux.dev>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2d6dfba540d4..683f9f9bf47e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3060,7 +3060,7 @@ static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
 
 	if (!local_trylock(&obj_stock.lock)) {
 		if (pgdat)
-			mod_objcg_mlstate(objcg, pgdat, idx, nr_bytes);
+			mod_objcg_mlstate(objcg, pgdat, idx, nr_acct);
 		nr_pages = nr_bytes >> PAGE_SHIFT;
 		nr_bytes = nr_bytes & (PAGE_SIZE - 1);
 		atomic_add(nr_bytes, &objcg->nr_charged_bytes);
-- 
2.50.1

Re: [PATCH] memcg: fix slab accounting in refill_obj_stock() trylock path

Posted by Johannes Weiner 1 month, 1 week ago

On Thu, Feb 26, 2026 at 07:51:37PM +0800, Hao Li wrote:
> In the trylock path of refill_obj_stock(), mod_objcg_mlstate() should
> use the real alloc/free bytes (i.e., nr_acct) for accounting, rather
> than nr_bytes.
> 
> Fixes: 200577f69f29 ("memcg: objcg stock trylock without irq disabling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hao Li <hao.li@linux.dev>

Oops. Yes, I suppose the contended case is quite rare (this is CPU
local), so I'm not surprised this went unnoticed for so long.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH] memcg: fix slab accounting in refill_obj_stock() trylock path

Posted by Shakeel Butt 1 month, 1 week ago

On Thu, Feb 26, 2026 at 07:51:37PM +0800, Hao Li wrote:
> In the trylock path of refill_obj_stock(), mod_objcg_mlstate() should
> use the real alloc/free bytes (i.e., nr_acct) for accounting, rather
> than nr_bytes.
> 
> Fixes: 200577f69f29 ("memcg: objcg stock trylock without irq disabling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hao Li <hao.li@linux.dev>

Thanks for the fix.

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Re: [PATCH] memcg: fix slab accounting in refill_obj_stock() trylock path

Posted by Vlastimil Babka 1 month, 1 week ago

On 2/26/26 14:39, Shakeel Butt wrote:
> On Thu, Feb 26, 2026 at 07:51:37PM +0800, Hao Li wrote:
>> In the trylock path of refill_obj_stock(), mod_objcg_mlstate() should
>> use the real alloc/free bytes (i.e., nr_acct) for accounting, rather
>> than nr_bytes.
>> 
>> Fixes: 200577f69f29 ("memcg: objcg stock trylock without irq disabling")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hao Li <hao.li@linux.dev>
> 
> Thanks for the fix.
> 
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

What are the user-visible effects of the bug?

Re: [PATCH] memcg: fix slab accounting in refill_obj_stock() trylock path

Posted by Hao Li 1 month, 1 week ago

On Thu, Feb 26, 2026 at 02:44:02PM +0100, Vlastimil Babka wrote:
> On 2/26/26 14:39, Shakeel Butt wrote:
> > On Thu, Feb 26, 2026 at 07:51:37PM +0800, Hao Li wrote:
> >> In the trylock path of refill_obj_stock(), mod_objcg_mlstate() should
> >> use the real alloc/free bytes (i.e., nr_acct) for accounting, rather
> >> than nr_bytes.
> >> 
> >> Fixes: 200577f69f29 ("memcg: objcg stock trylock without irq disabling")
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Hao Li <hao.li@linux.dev>
> > 
> > Thanks for the fix.
> > 
> > Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> 
> What are the user-visible effects of the bug?

The user-visible impact is that the NR_SLAB_RECLAIMABLE_B and
NR_SLAB_UNRECLAIMABLE_B stats can end up being incorrect.

For example, if a user allocates a 6144-byte object, then before this fix
refill_obj_stock() calls mod_objcg_mlstate(..., nr_bytes=2048), even though it
should account for 6144 bytes (i.e., nr_acct).

When the user later frees the same object with kfree(), refill_obj_stock() calls
mod_objcg_mlstate(..., nr_bytes=6144). This ends up adding 6144 to the stats,
but it should be applying -6144 (i.e., nr_acct) since the object is being
freed.

-- 
Thanks,
Hao

Re: [PATCH] memcg: fix slab accounting in refill_obj_stock() trylock path

Posted by Vlastimil Babka 1 month, 1 week ago

On 2/27/26 02:01, Hao Li wrote:
> On Thu, Feb 26, 2026 at 02:44:02PM +0100, Vlastimil Babka wrote:
>> On 2/26/26 14:39, Shakeel Butt wrote:
>> > On Thu, Feb 26, 2026 at 07:51:37PM +0800, Hao Li wrote:
>> >> In the trylock path of refill_obj_stock(), mod_objcg_mlstate() should
>> >> use the real alloc/free bytes (i.e., nr_acct) for accounting, rather
>> >> than nr_bytes.
>> >> 
>> >> Fixes: 200577f69f29 ("memcg: objcg stock trylock without irq disabling")
>> >> Cc: stable@vger.kernel.org
>> >> Signed-off-by: Hao Li <hao.li@linux.dev>
>> > 
>> > Thanks for the fix.
>> > 
>> > Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
>> 
>> What are the user-visible effects of the bug?
> 
> The user-visible impact is that the NR_SLAB_RECLAIMABLE_B and
> NR_SLAB_UNRECLAIMABLE_B stats can end up being incorrect.
> 
> For example, if a user allocates a 6144-byte object, then before this fix
> refill_obj_stock() calls mod_objcg_mlstate(..., nr_bytes=2048), even though it
> should account for 6144 bytes (i.e., nr_acct).
> 
> When the user later frees the same object with kfree(), refill_obj_stock() calls
> mod_objcg_mlstate(..., nr_bytes=6144). This ends up adding 6144 to the stats,
> but it should be applying -6144 (i.e., nr_acct) since the object is being
> freed.

Thanks, I'm sure Andrew will amend the changelog with those useful details.

Weird that we went since 6.16 with nobody noticing the stats were off - it
sounds they could get really way off?

Re: [PATCH] memcg: fix slab accounting in refill_obj_stock() trylock path

Posted by Hao Li 1 month, 1 week ago

On Fri, Feb 27, 2026 at 08:46:18AM +0100, Vlastimil Babka wrote:
> On 2/27/26 02:01, Hao Li wrote:
> > On Thu, Feb 26, 2026 at 02:44:02PM +0100, Vlastimil Babka wrote:
> >> On 2/26/26 14:39, Shakeel Butt wrote:
> >> > On Thu, Feb 26, 2026 at 07:51:37PM +0800, Hao Li wrote:
> >> >> In the trylock path of refill_obj_stock(), mod_objcg_mlstate() should
> >> >> use the real alloc/free bytes (i.e., nr_acct) for accounting, rather
> >> >> than nr_bytes.
> >> >> 
> >> >> Fixes: 200577f69f29 ("memcg: objcg stock trylock without irq disabling")
> >> >> Cc: stable@vger.kernel.org
> >> >> Signed-off-by: Hao Li <hao.li@linux.dev>
> >> > 
> >> > Thanks for the fix.
> >> > 
> >> > Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> >> 
> >> What are the user-visible effects of the bug?
> > 
> > The user-visible impact is that the NR_SLAB_RECLAIMABLE_B and
> > NR_SLAB_UNRECLAIMABLE_B stats can end up being incorrect.
> > 
> > For example, if a user allocates a 6144-byte object, then before this fix
> > refill_obj_stock() calls mod_objcg_mlstate(..., nr_bytes=2048), even though it
> > should account for 6144 bytes (i.e., nr_acct).
> > 
> > When the user later frees the same object with kfree(), refill_obj_stock() calls
> > mod_objcg_mlstate(..., nr_bytes=6144). This ends up adding 6144 to the stats,
> > but it should be applying -6144 (i.e., nr_acct) since the object is being
> > freed.
> 
> Thanks, I'm sure Andrew will amend the changelog with those useful details.

Got it. Thanks.

> 
> Weird that we went since 6.16 with nobody noticing the stats were off - it
> sounds they could get really way off?

Indeed, it does seem a bit unbelievable. I suspect the conditions required for
this issue to occur are quite strict: a process context first hold the
obj_stock.lock, then get interrupted by an IRQ, and the IRQ path also reach
refill_obj_stock and then hit the local_trylock-failed path.

Therefore, I think a small amount of data distortion might be hard to observe.

-- 
Thanks,
Hao

Re: [PATCH] memcg: fix slab accounting in refill_obj_stock() trylock path

Posted by Andrew Morton 1 month, 1 week ago

On Fri, 27 Feb 2026 16:37:16 +0800 Hao Li <hao.li@linux.dev> wrote:

> > Thanks, I'm sure Andrew will amend the changelog with those useful details.
> 
> Got it. Thanks.

I made that edit, thanks.