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

[Joshua Hahn <joshua.hahnjy@gmail.com>]

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