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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >  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