Re: [PATCH] mm: memcontrol: fix NULL pointer deref in high_work_func() on dying memcg

[Michal Hocko]

On Mon 26-10-26 11:01:51, T.J. Mercier wrote:
> On Mon, Oct 26, 2026 at 6:25 AM Michal Hocko <mhocko@xxxxxxxx> wrote:
> >
> > On Fri 23-10-26 15:23:13, T.J. Mercier wrote:
> > [...]
> > > ---
> > >  mm/memcontrol.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > > index a05d9a8baa1b..84f4a8fd4d16 100644
> > > --- a/mm/memcontrol.c
> > > +++ b/mm/memcontrol.c
> > > @@ -2457,7 +2457,10 @@ static void high_work_func(struct work_struct *work)
> > >       struct mem_cgroup *memcg;
> > >
> > >       memcg = container_of(work, struct mem_cgroup, high_work);
> > > +     if (!css_tryget_online(&memcg->css))
> > > +             return;
> > >       reclaim_high(memcg, MEMCG_CHARGE_BATCH, GFP_KERNEL);
> > > +     css_put(&memcg->css);
> > >  }
> >
> > OK.
> >
> > >  static void high_irq_work_func(struct irq_work *work)
> > > @@ -4463,6 +4466,8 @@ static void mem_cgroup_css_offline(struct cgroup_subsys_state *css)
> > >
> > >       page_counter_set_min(&memcg->memory, 0);
> > >       page_counter_set_low(&memcg->memory, 0);
> > > +     page_counter_set_high(&memcg->memory, PAGE_COUNTER_MAX);
> > > +     page_counter_set_high(&memcg->swap, PAGE_COUNTER_MAX);
> > >
> > >       zswap_memcg_offline_cleanup(memcg);
> >
> > Should we simply disable_work_sync when the memcg is going to be
> > released instead (mem_cgroup_css_released)? That should both prevent
> > the run reclaim_high during the memcg shut down and and also avoid
> > plaing with limits to work around.
> 
> Hi Michal,
> 
> Unfortunately that won't work for shrink_many() ->
> !mem_cgroup_tryget() -> lru_gen_release_memcg() which can occur before
> mem_cgroup_css_released() runs. (mem_cgroup_css_released() happens
> through the cgroup_release_wq.)

I do not follow. Could you explain why please?

> The high limit reset is also needed for targeting live ancestors above
> the offlined child in try_charge_memcg(). Without the resets,
> high_work is never attempted on them since it gets dropped when
> attempted on the offlined child due to the first hunk, and
> try_charge_memcg() breaks out of the loop.

I must be missing something really obvious here because I really do not
follow you here too. So we are offlining a memcg. We need to coordinate
memory reclaim with the memcg tear down, right? For the high reclaim we
can achieve this synchronization through disable_work_sync in
mem_cgroup_css_offline. For a synchronous high memory reclaim we do not
have such means but we are keeping a memcg reference which makes the
lifetime well defined. If you reset counter you might help to bail out
early(ier) but that shouldn't be necessary from a correctness POV as it
doesn't provide any explicit synchronization.

> Shakeel's remote charging patch
> (https://lore.kernel.org/all/20261010210957.1350874-4-shakeel.butt@xxxxxxxxx/)
> sends remote charges through high_work too, so without the resets
> those would lose ancestor reclaim the same way. Before that patch not
> performing the resets means reclaim_high() runs unnecesarily against
> current's memcg (not the remote memcg above memory.high).

-- 
Michal Hocko
SUSE Labs