Skip to contentSkip to conversation

memcg: fix slab accounting in refill_obj_stock() trylock path

PATCH Applied Hao Li 5 participants 8 messages Feb 26 – Feb 27

Subsystems (4)
Stable branch Memory management cgroup memcg
lore
Share & tools
lore

Hao Li

author
#0 lore raw

PATCH 1/1 memcg: fix slab accounting in refill_obj_stock() trylock path

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 [email protected]
Signed-off-by Hao Li <[email protected]>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
mm/memcontrol.c +1 -1
index 2d6dfba540d4..683f9f9bf47e 100644
@@ -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);
Signature · 1 line
2.50.1
#2 lore raw
Quoted 5 lines · Shakeel Butt
Quoted 4 lines · Hao Li

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.

Quoted 7 lines
Quoted 3 lines
Fixes 200577f69f29 ("memcg: objcg stock trylock without irq disabling")
Cc [email protected]
Signed-off-by Hao Li <[email protected]>

Thanks for the fix.

Acked-by Shakeel Butt <[email protected]>

What are the user-visible effects of the bug?

author
#3 lore raw
Quoted 10 lines · Vlastimil Babka
Quoted 9 lines · Shakeel Butt
Quoted 7 lines · Hao Li

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 [email protected]
Signed-off-by Hao Li <[email protected]>
Quoted 5 lines
Quoted 3 lines

Thanks for the fix.

Acked-by Shakeel Butt <[email protected]>

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.

Signature · 2 lines
Thanks,
Hao
#4 lore raw
Quoted 20 lines · Hao Li
Quoted 10 lines · Vlastimil Babka
Quoted 9 lines · Shakeel Butt
Quoted 7 lines · Hao Li

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 [email protected]
Signed-off-by Hao Li <[email protected]>
Quoted 5 lines
Quoted 3 lines

Thanks for the fix.

Acked-by Shakeel Butt <[email protected]>

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.

Quoted 8 lines

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?

author
#5 lore raw
Quoted 25 lines · Vlastimil Babka
Quoted 24 lines · Hao Li
Quoted 10 lines · Vlastimil Babka
Quoted 9 lines · Shakeel Butt
Quoted 7 lines · Hao Li

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 [email protected]
Signed-off-by Hao Li <[email protected]>
Quoted 5 lines
Quoted 3 lines

Thanks for the fix.

Acked-by Shakeel Butt <[email protected]>

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

Quoted 6 lines
Quoted 4 lines

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.

Quoted 3 lines

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.

Signature · 2 lines
Thanks,
Hao
maintainer
#6 lore raw
Quoted 3 lines · On Fri, 27 Feb 2026 16:37:16 +0800 Hao Li <[email protected]> wrote:
Quoted 1 line

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

Got it. Thanks.

I made that edit, thanks.

maintainer Acked-by on v1 1/1
#7 lore raw
In reply to Hao Li
Quoted 7 lines · Hao Li

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 [email protected]
Signed-off-by Hao Li <[email protected]>

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 <[email protected]>