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

[T.J. Mercier]

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.)

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.

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).