Re: [next-20260216]NULL pointer dereference in drain_obj_stock() (RCU free path)

[Harry Yoo <harry.yoo@oracle.com>]

On Sun, Feb 22, 2026 at 03:48:53PM -0800, Shakeel Butt wrote:
> On Sun, Feb 22, 2026 at 03:36:46PM -0800, Shakeel Butt wrote:
> > On Sun, Feb 22, 2026 at 08:47:03PM +0900, Harry Yoo wrote:
> > [...]
> > > 
> > > It seems it crashed while dereferencing objcg->ref->data->count.
> > > I think that implies that obj_cgroup_release()->percpu_ref_exit()
> > > is already called due to the refcount reaching zero and set
> > > ref->data = NULL.
> > > 
> > > Wait, was the stock->objcg ever a valid objcg?
> > > I think it should be valid when refilling the obj stock, otherwise
> > > it should have crashed in refill_obj_stock() -> obj_cgroup_get() path
> > > in the first place, rather than crashing when draining.
> > > 
> > > And that sounds like we're somehow calling obj_cgroup_put() more times
> > > than obj_cgroup_get().
> > > 
> > > Anyway, this is my theory that it may be due to mis-refcounting of objcgs.
> > > 
> > 
> > I have not looked deeper into recent slub changes (sheafs or obj_exts savings)
> > but one thing looks weird to me:
> > 
> > allocate_slab() // for cache with SLAB_OBJ_EXT_IN_OBJ
> > 	-> alloc_slab_obj_exts_early()
> > 		-> slab_set_stride(slab, s->size)
> > 	-> account_slab()
> > 		-> alloc_slab_obj_exts()
> > 			-> slab_set_stride(slab, sizeof(struct slabobj_ext));
> > 
> > Unconditional overwrite of stride. Not sure if it is issue or even related to
> > this crash but looks odd.
> 
> I asked AI to debug this crash report along with a nudge towards to look for
> stride corruption, it gave me the following output:

Thanks!

TLDR; I think stride corruption cannot happen because we don't try to
allocate obj_exts if we already have it (w/ SLAB_OBJ_EXT_IN_OBJ).

But the other analysis saying it's memory ordering issue seems
very relevant.

> # Stride Corruption Bug Analysis
> 
> ## Bug Report Context
> 
> - **Crash Location**: `drain_obj_stock+0x620/0xa48` in `obj_cgroup_put(old)` at mm/memcontrol.c:3059
> - **Root Cause**: `percpu_ref.data` is NULL, meaning `obj_cgroup_release()` already ran
> - **Platform**: IBM Power11 (pSeries LPAR, Radix MMU, LE, 64K pages, kernel 6.19.0-next-20260216)
> - **Trigger**: xfstests generic/428
> 
> ## Identified Bug: Unconditional Stride Overwrite
> 
> ### Location: mm/slub.c lines 2196-2223 (alloc_slab_obj_exts)
> 
> ```c
> retry:
>     old_exts = READ_ONCE(slab->obj_exts);
>     handle_failed_objexts_alloc(old_exts, vec, objects);
>     slab_set_stride(slab, sizeof(struct slabobj_ext));  // BUG: UNCONDITIONALLY SET
> 
>     if (new_slab) {
>         slab->obj_exts = new_exts;
>     } else if (old_exts & ~OBJEXTS_FLAGS_MASK) {
>         // obj_exts already exists, BUT stride was already modified above!
>         mark_objexts_empty(vec);
>         kfree(vec);
>         return 0;
>     } else if (cmpxchg(&slab->obj_exts, old_exts, new_exts) != old_exts) {
>         goto retry;
>     }
> ```
> 
> ### The Problem
> 
> The stride is set to `sizeof(struct slabobj_ext)` **BEFORE** checking if `obj_exts` already
> exists. If a slab was created with `SLAB_OBJ_EXT_IN_OBJ` mode (where stride = `s->size`),
> and later `alloc_slab_obj_exts` is called for any reason, the stride gets corrupted.

I think this part is bogus.

It's not a bug because obj_exts saving is applied only during slab
allocation and initialization. If it benefits from the obj_exts savings,
no need to call alloc_slab_obj_exts() later, thus no corruption.

If doesn't benefit from the obj_exts savings, the stride size is always
sizeof(struct slabobj_ext).

> ### Stride Modes
> 
> There are two stride modes (see alloc_slab_obj_exts_early):
> 
> 1. **Normal mode**: stride = `sizeof(struct slabobj_ext)` (~16 bytes)
>    - obj_exts is a separate array or in slab leftover space
> 
> 2. **SLAB_OBJ_EXT_IN_OBJ mode**: stride = `s->size` (object size, e.g., 64-256+ bytes)
>    - obj_ext is embedded within each object at a fixed offset
> 
> ### Consequences of Wrong Stride
> 
> When `slab_obj_ext` is later called:
> ```c
> obj_ext = (struct slabobj_ext *)(obj_exts + slab_get_stride(slab) * index);
> ```
> 
> With corrupted stride (16 instead of 256):
> - **Expected**: `obj_exts + 256 * 5 = obj_exts + 1280` (correct obj_ext for object 5)
> - **Actual**: `obj_exts + 16 * 5 = obj_exts + 80` (WRONG obj_ext - belongs to object 0!)

[...snip...]

> ## CRITICAL: Memory Ordering Bug on PowerPC (Likely Root Cause)
> 
> ### The Problem
> 
> In `alloc_slab_obj_exts` (mm/slub.c lines 2199-2220), there is **NO memory barrier**
> between the stride store and the obj_exts visibility via cmpxchg:

This is actually a good point.
 
> ```c
> slab_set_stride(slab, sizeof(struct slabobj_ext));  // Store to stride (line 2199)
>                                                      // NO MEMORY BARRIER HERE!
> if (new_slab) {
>     slab->obj_exts = new_exts;                       // Store to obj_exts (line 2207)
> } else if (...) {
> } else if (cmpxchg(&slab->obj_exts, ...) != ...) {   // Atomic on obj_exts (line 2220)
>     goto retry;
> }
> ```
>
> ### Why This Crashes on PowerPC
> 
> PowerPC has a **weakly-ordered memory model**. Stores can be reordered and may not be
> immediately visible to other processors. The cmpxchg provides a barrier AFTER it
> executes, but the stride store BEFORE cmpxchg may not be visible when obj_exts becomes
> visible.
> 
> **Race Scenario:**
> 1. CPU A: `slab_set_stride(slab, 16)` (store to stride, in CPU A's store buffer)
> 2. CPU A: `cmpxchg(&slab->obj_exts, 0, new_exts)` succeeds, obj_exts is now visible
> 3. CPU B: Sees `obj_exts` is set (from step 2)
> 4. CPU B: Reads `slab->stride` → **sees OLD value (0 or garbage)** due to reordering!
> 5. CPU B: `slab_obj_ext` calculates `obj_exts + 0 * index = obj_exts` for ALL indices!
> 6. **All objects appear to share the same obj_ext at offset 0**

Yes, that could actually happen, especially when the cache doesn't
specify SLAB_ACCOUNT but allocate objects with __GFP_ACCOUNT set
(e.g. xarray does that).

With sheaves for all, objects can be in different CPUs' sheaves and they
could try to allocate obj_exts and charge objects from the same slab.

> ### Consequences
> 
> - Object 0's objcg is correct
> - Object 1..N all read object 0's objcg (WRONG!)
> - When freeing multiple objects, we `obj_cgroup_put` the SAME objcg multiple times

Yes, reading the wrong stride value is the path to mis-refcounting
objcgs :P

> - Reference count underflows → early `obj_cgroup_release()`
> - `percpu_ref_exit()` sets `data = NULL`
> - Later stock drain tries to put the objcg → **CRASH in `drain_obj_stock`**

> ### Why This Matches the Bug Report
> 
> - **Platform**: IBM Power11 (PowerPC) - weakly ordered memory
> - **Trigger**: xfstests generic/428 - creates high filesystem activity with many allocations
> - **Crash location**: `drain_obj_stock` → `obj_cgroup_put` with NULL data
> - **Pattern**: Stock has cached objcg that was prematurely released
> 
> ### Proposed Fix
> 
> Add a write memory barrier to ensure stride is visible before obj_exts:
> 
> ```c
> slab_set_stride(slab, sizeof(struct slabobj_ext));
> smp_wmb();  // Ensure stride is visible before obj_exts
> 
> if (new_slab) {
>     slab->obj_exts = new_exts;
> } else if (...) {
> } else if (cmpxchg(&slab->obj_exts, old_exts, new_exts) != old_exts) {
>     goto retry;
> }
> ```

I would say this is an overkill. Something like this should suffice:

diff --git a/mm/slub.c b/mm/slub.c
index 865bc050f654..8db93a88bbbf 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2196,7 +2196,6 @@ int alloc_slab_obj_exts(struct slab *slab, struct kmem_cache *s,
 retry:
 	old_exts = READ_ONCE(slab->obj_exts);
 	handle_failed_objexts_alloc(old_exts, vec, objects);
-	slab_set_stride(slab, sizeof(struct slabobj_ext));

 	if (new_slab) {
 		/*
@@ -2272,6 +2271,12 @@ static void alloc_slab_obj_exts_early(struct kmem_cache *s, struct slab *slab)
 	void *addr;
 	unsigned long obj_exts;

+	/*
+	 * Initialize stride unconditionally,
+	 * but override if SLAB_OBJ_EXT_IN_OBJ is set.
+	 */
+	slab_set_stride(slab, sizeof(struct slabobj_ext));
+
 	if (!need_slab_obj_exts(s))
 		return;

@@ -2288,7 +2293,6 @@ static void alloc_slab_obj_exts_early(struct kmem_cache *s, struct slab *slab)
 		obj_exts |= MEMCG_DATA_OBJEXTS;
 #endif
 		slab->obj_exts = obj_exts;
-		slab_set_stride(slab, sizeof(struct slabobj_ext));
 	} else if (s->flags & SLAB_OBJ_EXT_IN_OBJ) {
 		unsigned int offset = obj_exts_offset_in_object(s);

There should be enough memory barriers to ensure the stride and obj_exts
are visible to other CPUs before the slab is accessible by other CPUs
(by acquiring/releasing n->list_lock)

> And correspondingly, the reader side should use a read barrier:
> ```c
> static inline unsigned short slab_get_stride(struct slab *slab)
> {
>     // Need acquire semantics when reading stride after seeing obj_exts
>     return smp_load_acquire(&slab->stride);
> }
> ```
> 
> Or use `smp_store_release` / `smp_load_acquire` pairs for proper ordering.
> 
> ### Also Applies to alloc_slab_obj_exts_early
> 
> The same issue exists in `alloc_slab_obj_exts_early` (lines 2290-2291 and 2308-2309):
> 
> ```c
> slab->obj_exts = obj_exts;                           // Store obj_exts
> slab_set_stride(slab, sizeof(struct slabobj_ext));   // Store stride AFTER!
> ```
> 
> Here the order is **reversed** - obj_exts is set BEFORE stride! This is even worse
> for memory ordering, as other CPUs could see obj_exts before stride is set.

[...]

-- 
Cheers,
Harry / Hyeonggon