Re: [PATCH] mm/memcontrol: publish null pointer before dropping css reference

[Joshua Hahn]

On Tue, 20 Oct 2026 14:21:22 -0700 Shakeel Butt <shakeel.butt@xxxxxxxxx> wrote:

> On Tue, Oct 20, 2026 at 12:35:31PM -0700, Joshua Hahn wrote:
> > On Tue, 20 Oct 2026 12:03:38 -0700 Joshua Hahn <joshua.hahnjy@xxxxxxxxx> wrote:
> > 
> 
> [...]
> 
> > > 
> > > So for drain_local_memcg_stock I agree, since FLUSHING_CACHED_CHARGE
> > > is set, so you can't have it concurrent with drain_all_stock since it
> > > will skip that stock. But refill_stock also calls drain_stock when
> > > it evicts the slot but the flag isn't set, so is_memcg_drain_needed
> > > can read it at the same time.
> > > 
> > > What do you think I should do? I think reordering makes the intent more
> > > obvious and maybe it's just me but I did find it a bit misleading at
> > > first that we were dropping the reference and then publishing. 
> > 
> > Oh and just for clarification, my page_counter_stock series actually
> > gets rid of this codepath and switches the order when moving it anyways.
> 
> Yes but this patch has fixes and might get backported, so we need to be use it
> is fixing an issue which is real.
> Let's try to reason how the race can happen between is_memcg_drain_needed and
> drain_stock:
> 
> CPU 1						CPU 2
> 
> drain_stock
>   WRITE_ONCE(stock->nr_pages[i], 0);		is_memcg_drain_needed
>   css_put(&old->css);
>   /* assume full grace period */
>   						  rcu_read_lock()
>   						  memcg = READ_ONCE(stock->cached[i]);
>   						  if(READ_ONCE(stock->nr_pages[i])
>   							mem_cgroup_is_descendant(memcg);
> 
>   WRITE_ONCE(stock->cached[i], NULL);
> 
> 
> Do you think there is possible race condition in above? I couldn't see.
> 
> However I ran this through AI and it has convinced me that on PREEMPT_RT, this
> is indeed racy access.

Wow, thank you for going through and doing an analysis!

> AI reasoning:
> -------------
> 
> How it breaks on PREEMPT_RT
> 
> On RT, local_trylock() takes a per-CPU spinlock_t (an rtmutex). That is
> rcu_read_lock() plus migrate_disable(), but the task can still be preempted
> inside it.
> 
> 1. CPU A, refill_stock(X): there is no free slot, so it calls drain_stock(i) on
>    old. It sets nr_pages[i] = 0, then css_put(old) drops the last reference and
>    queues the release work on CPU A.
> 2. The refill task is preempted (by a tick or a threaded IRQ) before it clears
>    cached[i]. CPU A's kworker runs css_release_work_fn(), which calls call_rcu(),
>    and grace period G starts. G will wait for the refill task, because that task is
>    inside an RCU reader.
> 3. CPU B, drain_all_stock() → is_memcg_drain_needed(): B has already passed a
>    quiescent state for G. It enters rcu_read_lock() and loads cached[i] == old. G
>    does not wait for this reader, because it started after G did.
> 4. B is preempted right after that load (RT readers can be preempted).
> 5. A resumes. It clears cached[i], puts X in slot i with a non-zero nr_pages[i],
>    and unlocks. G ends, and old is freed.
> 6. B resumes. It reads X's non-zero count and calls
>    mem_cgroup_is_descendant(old, root). That reads old->css.cgroup from freed
>    memory.

Thanks for taking a look. I think this is the race condition that Sashiko
brought up in the original series that I had.

I will go ahead and send a v2 with a new commit message that explains
this PREEMPT_RT race condition so that it is more obvious where it can
actually race in a kernel. Thanks!

Joshua