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

[T.J. Mercier]

On Tue, Oct 27, 2026 at 2:41 AM Michal Hocko <mhocko@xxxxxxxx> wrote:
>
> 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?

Calling disable_work_sync in mem_cgroup_css_released() is too late. As
soon as the memcg refcount drops to 0, mem_cgroup_tryget() starts
failing, and lru_gen_release_memcg() can run.
mem_cgroup_css_released() runs async via css_release() ->
css_release_work_fn().

high_work_func() doesn't hold a memcg reference, so the memcg refcount
can hit 0 while high_work_func() is already running, or waiting to
run.

high_work_func()                    kswapd
reclaim_high(memcg)
 try_to_free_mem_cgroup_pages()
                                    <memcg refcount hits 0>
                                    shrink_many()
                                     mem_cgroup_tryget() fails
                                     lru_gen_release_memcg()
  shrink_lruvec()
   lru_gen_rotate_memcg()
    NULL pprev deref

... later, on cgroup_release_wq:
css_release_work_fn()
 mem_cgroup_css_released()
  disable_work_sync()   /* too late */

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

We could call disable_work_sync() in mem_cgroup_css_offline(), but
that waits until high_work_func() is done and it could be performing
reclaim. mem_cgroup_css_offline() runs under the cgroup_mutex, and
cgroup_mutex contention is already bad enough. I don't want to add to
it. It would also block the rest of the cgroup_offline_wq.

Every other caller of try_to_free_mem_cgroup_pages() keeps the memcg
alive. high_work_func() should too.

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

The high resets are not for synchronization. They are for performing
high_work on the correct live memcg if necessary.

Even if we take the disable_work_sync() approach, we still need the
high limit resets. Any try_charge_memcg() on that offlined memcg would
break out of this loop too early at schedule_work(&memcg->high_work)
(a no-op), and potentially skip calling it on any of the parents.

do {
        bool mem_high, swap_high;
        mem_high = page_counter_read(&memcg->memory) >
                READ_ONCE(memcg->memory.high);
        swap_high = page_counter_read(&memcg->swap) >
                READ_ONCE(memcg->swap.high);

        /* Don't bother a random interrupted task */
        if (!in_task()) {
                if (mem_high) {
                        if (allow_spinning)
                                schedule_work(&memcg->high_work);
                        else
                                irq_work_queue(&memcg->high_irq_work);
                        break;
                }
                continue;
        }
...
} while ((memcg = parent_mem_cgroup(memcg)));


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