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

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

[Joshua Hahn]

struct memcg_stock_pcp hold references to cgroups that it tracks to make
sure that the memcg never gets freed with unaccounted charges still
living in the stock. The cached stock can be cleared either via
drain_stock() or consume_stock(), which drop the reference if the
stock goes to 0, and then clears the pointer from the stock.

If that was the last reference, the memcg is freed after an RCU grace
period, but this can happen while the pointer still points to the
to-be-freed memcg. Then a reader that enters its read-side critical
section after the grace period has started is not waited for, and then
can dereference it after the memcg gets freed.

In practice, this is very rare and I have not been able to reproduce
the issue in my own testing.

Switch the order of the reference drop and the publishing so that
we first publish the NULL pointer, then drop the reference.

This is based on a report from Sashiko, who found this pre-existing
issue in my page_counter_stock series [1].

[1] https://sashiko.dev/#/patchset/20260928192349.3432886-1-joshua.hahnjy%40gmail.com

Fixes: e1a366be5cb4 ("mm: memcontrol: switch to rcu protection in drain_all_stock()")
Signed-off-by: Joshua Hahn <joshua.hahnjy@gmail.com>
---
 mm/memcontrol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index e7d7db5718f9a..a05d9a8baa1b6 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2172,8 +2172,8 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
 			stock_pages -= nr_pages;
 			WRITE_ONCE(stock->nr_pages[i], stock_pages);
 			if (!stock_pages) {
-				css_put(&memcg->css);
 				WRITE_ONCE(stock->cached[i], NULL);
+				css_put(&memcg->css);
 			}
 			ret = true;
 		}
@@ -2209,8 +2209,8 @@ static void drain_stock(struct memcg_stock_pcp *stock, int i)
 		WRITE_ONCE(stock->nr_pages[i], 0);
 	}
 
-	css_put(&old->css);
 	WRITE_ONCE(stock->cached[i], NULL);
+	css_put(&old->css);
 }
 
 static void drain_stock_fully(struct memcg_stock_pcp *stock)
-- 
2.53.0-Meta

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

[Shakeel Butt]

On Tue, Oct 20, 2026 at 09:56:26AM -0700, Joshua Hahn wrote:
> struct memcg_stock_pcp hold references to cgroups that it tracks to make
> sure that the memcg never gets freed with unaccounted charges still
> living in the stock. The cached stock can be cleared either via
> drain_stock() or consume_stock(), which drop the reference if the
> stock goes to 0, and then clears the pointer from the stock.
> 
> If that was the last reference, the memcg is freed after an RCU grace
> period, but this can happen while the pointer still points to the
> to-be-freed memcg. Then a reader that enters its read-side critical
> section after the grace period has started is not waited for, and then
> can dereference it after the memcg gets freed.

These are per-cpu fields and are accessed within local_lock. How can there be a
reader where a cpu is still between css_put and WRITE_ONCE and within
local_lock?

> 
> In practice, this is very rare and I have not been able to reproduce
> the issue in my own testing.
> 
> Switch the order of the reference drop and the publishing so that
> we first publish the NULL pointer, then drop the reference.
> 
> This is based on a report from Sashiko, who found this pre-existing
> issue in my page_counter_stock series [1].
> 
> [1] https://sashiko.dev/#/patchset/20260928192349.3432886-1-joshua.hahnjy%40gmail.com
> 
> Fixes: e1a366be5cb4 ("mm: memcontrol: switch to rcu protection in drain_all_stock()")
> Signed-off-by: Joshua Hahn <joshua.hahnjy@gmail.com>
> ---
>  mm/memcontrol.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index e7d7db5718f9a..a05d9a8baa1b6 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2172,8 +2172,8 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
>  			stock_pages -= nr_pages;
>  			WRITE_ONCE(stock->nr_pages[i], stock_pages);
>  			if (!stock_pages) {
> -				css_put(&memcg->css);
>  				WRITE_ONCE(stock->cached[i], NULL);
> +				css_put(&memcg->css);

This css_put can not be the last reference for the memcg from this path as the
caller of consume_stock() (above the stack) should be holding a reference to the
memcg.

>  			}
>  			ret = true;
>  		}
> @@ -2209,8 +2209,8 @@ static void drain_stock(struct memcg_stock_pcp *stock, int i)
>  		WRITE_ONCE(stock->nr_pages[i], 0);
>  	}
>  
> -	css_put(&old->css);
>  	WRITE_ONCE(stock->cached[i], NULL);
> +	css_put(&old->css);

Here this can be last reference but I don't see any concurrent readers of stock.

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

[Joshua Hahn]

On Tue, 20 Oct 2026 11:13:27 -0700 Shakeel Butt <shakeel.butt@linux.dev> wrote:

> On Tue, Oct 20, 2026 at 09:56:26AM -0700, Joshua Hahn wrote:
> > struct memcg_stock_pcp hold references to cgroups that it tracks to make
> > sure that the memcg never gets freed with unaccounted charges still
> > living in the stock. The cached stock can be cleared either via
> > drain_stock() or consume_stock(), which drop the reference if the
> > stock goes to 0, and then clears the pointer from the stock.
> > 
> > If that was the last reference, the memcg is freed after an RCU grace
> > period, but this can happen while the pointer still points to the
> > to-be-freed memcg. Then a reader that enters its read-side critical
> > section after the grace period has started is not waited for, and then
> > can dereference it after the memcg gets freed.

Hi Shakeel,

Thank you for the very quick review! : -D

> These are per-cpu fields and are accessed within local_lock. How can there be a
> reader where a cpu is still between css_put and WRITE_ONCE and within
> local_lock?

I think there is a case where a different CPU can actually access the
percpu stock. drain_all_stock walks every online CPU's memcg_stock
and calls is_memcg_drain_needed, which reads the value with READ_ONCE
with no local_lock. So I think there can be a concurrent reader that
doesn't need the local lock. 
 
> > In practice, this is very rare and I have not been able to reproduce
> > the issue in my own testing.
> > 
> > Switch the order of the reference drop and the publishing so that
> > we first publish the NULL pointer, then drop the reference.
> > 
> > This is based on a report from Sashiko, who found this pre-existing
> > issue in my page_counter_stock series [1].
> > 
> > [1] https://sashiko.dev/#/patchset/20260928192349.3432886-1-joshua.hahnjy%40gmail.com
> > 
> > Fixes: e1a366be5cb4 ("mm: memcontrol: switch to rcu protection in drain_all_stock()")
> > Signed-off-by: Joshua Hahn <joshua.hahnjy@gmail.com>
> > ---
> >  mm/memcontrol.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index e7d7db5718f9a..a05d9a8baa1b6 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2172,8 +2172,8 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
> >  			stock_pages -= nr_pages;
> >  			WRITE_ONCE(stock->nr_pages[i], stock_pages);
> >  			if (!stock_pages) {
> > -				css_put(&memcg->css);
> >  				WRITE_ONCE(stock->cached[i], NULL);
> > +				css_put(&memcg->css);
> 
> This css_put can not be the last reference for the memcg from this path as the
> caller of consume_stock() (above the stack) should be holding a reference to the
> memcg.

Ah right, that's true!

> >  			}
> >  			ret = true;
> >  		}
> > @@ -2209,8 +2209,8 @@ static void drain_stock(struct memcg_stock_pcp *stock, int i)
> >  		WRITE_ONCE(stock->nr_pages[i], 0);
> >  	}
> >  
> > -	css_put(&old->css);
> >  	WRITE_ONCE(stock->cached[i], NULL);
> > +	css_put(&old->css);
> 
> Here this can be last reference but I don't see any concurrent readers of stock.

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. 

Thanks again Shakeel. Have a great day!!
Joshua

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

[Joshua Hahn]

On Tue, 20 Oct 2026 12:03:38 -0700 Joshua Hahn <joshua.hahnjy@gmail.com> wrote:

> On Tue, 20 Oct 2026 11:13:27 -0700 Shakeel Butt <shakeel.butt@linux.dev> wrote:
> 
> > On Tue, Oct 20, 2026 at 09:56:26AM -0700, Joshua Hahn wrote:
> > > struct memcg_stock_pcp hold references to cgroups that it tracks to make
> > > sure that the memcg never gets freed with unaccounted charges still
> > > living in the stock. The cached stock can be cleared either via
> > > drain_stock() or consume_stock(), which drop the reference if the
> > > stock goes to 0, and then clears the pointer from the stock.
> > > 
> > > If that was the last reference, the memcg is freed after an RCU grace
> > > period, but this can happen while the pointer still points to the
> > > to-be-freed memcg. Then a reader that enters its read-side critical
> > > section after the grace period has started is not waited for, and then
> > > can dereference it after the memcg gets freed.
> 
> Hi Shakeel,
> 
> Thank you for the very quick review! : -D
> 
> > These are per-cpu fields and are accessed within local_lock. How can there be a
> > reader where a cpu is still between css_put and WRITE_ONCE and within
> > local_lock?
> 
> I think there is a case where a different CPU can actually access the
> percpu stock. drain_all_stock walks every online CPU's memcg_stock
> and calls is_memcg_drain_needed, which reads the value with READ_ONCE
> with no local_lock. So I think there can be a concurrent reader that
> doesn't need the local lock. 
>  
> > > In practice, this is very rare and I have not been able to reproduce
> > > the issue in my own testing.
> > > 
> > > Switch the order of the reference drop and the publishing so that
> > > we first publish the NULL pointer, then drop the reference.
> > > 
> > > This is based on a report from Sashiko, who found this pre-existing
> > > issue in my page_counter_stock series [1].
> > > 
> > > [1] https://sashiko.dev/#/patchset/20260928192349.3432886-1-joshua.hahnjy%40gmail.com
> > > 
> > > Fixes: e1a366be5cb4 ("mm: memcontrol: switch to rcu protection in drain_all_stock()")
> > > Signed-off-by: Joshua Hahn <joshua.hahnjy@gmail.com>
> > > ---
> > >  mm/memcontrol.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index e7d7db5718f9a..a05d9a8baa1b6 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -2172,8 +2172,8 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
> > >  			stock_pages -= nr_pages;
> > >  			WRITE_ONCE(stock->nr_pages[i], stock_pages);
> > >  			if (!stock_pages) {
> > > -				css_put(&memcg->css);
> > >  				WRITE_ONCE(stock->cached[i], NULL);
> > > +				css_put(&memcg->css);
> > 
> > This css_put can not be the last reference for the memcg from this path as the
> > caller of consume_stock() (above the stack) should be holding a reference to the
> > memcg.
> 
> Ah right, that's true!
> 
> > >  			}
> > >  			ret = true;
> > >  		}
> > > @@ -2209,8 +2209,8 @@ static void drain_stock(struct memcg_stock_pcp *stock, int i)
> > >  		WRITE_ONCE(stock->nr_pages[i], 0);
> > >  	}
> > >  
> > > -	css_put(&old->css);
> > >  	WRITE_ONCE(stock->cached[i], NULL);
> > > +	css_put(&old->css);
> > 
> > Here this can be last reference but I don't see any concurrent readers of stock.
> 
> 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.
I just thought I would pull this out early and send it as a fix to see
if it would be worth the effort!

> Thanks again Shakeel. Have a great day!!
> Joshua

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

[Shakeel Butt]

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@gmail.com> 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.

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.

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@linux.dev> 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@gmail.com> 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