[PATCH] mm: list_lru: don't use a new memcg's lists for an offline memcg

[PATCH] mm: list_lru: don't use a new memcg's lists for an offline memcg

[Shakeel Butt]

A memcg's list_lru lists are found by its kmemcg_id, which is its
private ID. Offlining a memcg moves its lists to the parent and frees
the ID right away, with no RCU grace period. The next new memcg can get
the same ID and allocate its own lists at that index.

A caller that holds only the RCU read lock, such as
__folio_unqueue_deferred_split(), can still hold the offline memcg. If
it is preempted until the ID is reused, it then locks the new memcg's
list and unlinks a folio that now sits on the parent's list, without
holding the parent's lock. A concurrent change to the parent's list can
then leave a pointer to a freed folio on it.

Users see list corruption reports, crashes or silent memory corruption
when memory cgroups are removed and created while THPs are freed, on
kernels with preemptible RCU, which is the default on x86 and arm64.
They can also see a warning from memcg_reparent_list_lru_one() when the
new memcg is removed, because the wrong unlink made its count negative.

Fix it by recording the owning memcg in each list_lru_memcg, and by
treating the index as empty when its lists belong to another memcg. The
lookup then moves up to the parent, whose list holds the folio.

Fixes: 28e98022b31e ("mm/list_lru: simplify reparenting and initial allocation")
Reported-by: Chris Mason <mason@kernel.org>
Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 include/linux/list_lru.h |  2 ++
 mm/list_lru.c            | 18 +++++++++++-------
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
index a450fffe1550..6ef5bc539606 100644
--- a/include/linux/list_lru.h
+++ b/include/linux/list_lru.h
@@ -38,6 +38,8 @@ struct list_lru_one {
 
 struct list_lru_memcg {
 	struct rcu_head		rcu;
+	/* owning memcg, only compared and never dereferenced */
+	const void		*memcg;
 	/* array of per cgroup per node lists, indexed by node id */
 	struct list_lru_one	node[];
 };
diff --git a/mm/list_lru.c b/mm/list_lru.c
index ca8de3793166..560190d5b348 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -72,12 +72,15 @@ static int lru_shrinker_id(struct list_lru *lru)
 }
 
 static inline struct list_lru_one *
-list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
+list_lru_from_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg)
 {
+	int idx = memcg_kmem_id(memcg);
+
 	if (list_lru_memcg_aware(lru) && idx >= 0) {
 		struct list_lru_memcg *mlru = xa_load(&lru->xa, idx);
 
-		return mlru ? &mlru->node[nid] : NULL;
+		/* Lists at a reused index belong to another memcg. */
+		return mlru && mlru->memcg == memcg ? &mlru->node[nid] : NULL;
 	}
 	return &lru->node[nid].lru;
 }
@@ -91,7 +94,7 @@ lock_list_lru_of_memcg(struct list_lru *lru, int nid,
 
 	rcu_read_lock();
 again:
-	l = list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(*memcg));
+	l = list_lru_from_memcg(lru, nid, *memcg);
 	if (likely(l)) {
 		lock_list_lru(l, irq, irq_flags);
 		if (likely(READ_ONCE(l->nr_items) != LONG_MIN)) {
@@ -132,7 +135,7 @@ static inline bool list_lru_memcg_aware(struct list_lru *lru)
 }
 
 static inline struct list_lru_one *
-list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
+list_lru_from_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg)
 {
 	return &lru->node[nid].lru;
 }
@@ -313,7 +316,7 @@ unsigned long list_lru_count_one(struct list_lru *lru,
 	long count;
 
 	rcu_read_lock();
-	l = list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(memcg));
+	l = list_lru_from_memcg(lru, nid, memcg);
 	count = l ? READ_ONCE(l->nr_items) : 0;
 	rcu_read_unlock();
 
@@ -502,11 +505,10 @@ static void memcg_reparent_list_lru_one(struct list_lru *lru, int nid,
 					struct list_lru_one *src,
 					struct mem_cgroup *dst_memcg)
 {
-	int dst_idx = dst_memcg->kmemcg_id;
 	struct list_lru_one *dst;
 
 	spin_lock_irq(&src->lock);
-	dst = list_lru_from_memcg_idx(lru, nid, dst_idx);
+	dst = list_lru_from_memcg(lru, nid, dst_memcg);
 	spin_lock_nested(&dst->lock, SINGLE_DEPTH_NESTING);
 
 	list_splice_init(&src->list, &dst->list);
@@ -602,6 +604,8 @@ static int __memcg_list_lru_alloc(struct mem_cgroup *memcg,
 			if (!mlru)
 				return -ENOMEM;
 		}
+		/* Set before the lists are published, and never changed after. */
+		mlru->memcg = pos;
 		xas_set(&xas, pos->kmemcg_id);
 		do {
 			xas_lock_irqsave(&xas, flags);
-- 
2.53.0-Meta

Re: [PATCH] mm: list_lru: don't use a new memcg's lists for an offline memcg

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] mm: list_lru: missing mlru->memcg check in
  memcg_list_lru_allocated()
--

commit d213b82b7b4ca4978c858df10a2a8cdea14626e4
Author: Shakeel Butt <shakeel.butt@linux.dev>

mm: list_lru: don't use a new memcg's lists for an offline memcg

This commit records the owning memcg in struct list_lru_memcg and checks it
in list_lru_from_memcg() so that an offline memcg whose kmemcg_id was reused
does not use the new memcg's lists.

> diff --git a/mm/list_lru.c b/mm/list_lru.c
> index d1d256822ccc7..937dbf239c9d4 100644
> --- a/mm/list_lru.c
> +++ b/mm/list_lru.c
> @@ -72,12 +72,15 @@ static int lru_shrinker_id(struct list_lru *lru)
>  }
>  
>  static inline struct list_lru_one *
> -list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
> +list_lru_from_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg)
>  {
> +	int idx = memcg_kmem_id(memcg);
> +
>  	if (list_lru_memcg_aware(lru) && idx >= 0) {
>  		struct list_lru_memcg *mlru = xa_load(&lru->xa, idx);
>  
> -		return mlru ? &mlru->node[nid] : NULL;
> +		/* Lists at a reused index belong to another memcg. */
> +		return mlru && mlru->memcg == memcg ? &mlru->node[nid] : NULL;
>  	}
>  	return &lru->node[nid].lru;
>  }

[ ... ]

> @@ -602,6 +604,8 @@ static int __memcg_list_lru_alloc(struct mem_cgroup *memcg,
>  			if (!mlru)
>  				return -ENOMEM;
>  		}
> +		/* Set before the lists are published, and never changed after. */
> +		mlru->memcg = pos;
>  		xas_set(&xas, pos->kmemcg_id);
>  		do {
>  			xas_lock_irqsave(&xas, flags);

[Severity: High]
Does memcg_list_lru_allocated() also need to check mlru->memcg == memcg?

mm/list_lru.c:memcg_list_lru_allocated() {
	int idx = memcg->kmemcg_id;

	return idx < 0 || xa_load(&lru->xa, idx);
}

Consider a hierarchy root -> P -> C where P is online and neither P nor C
has allocated a list_lru_memcg in lru->xa yet:

CPU0                                  CPU1
folio_memcg_list_lru_alloc(folio, lru)
  rcu_read_lock()
  memcg = folio_memcg(folio)  /* C */
                                      mem_cgroup_css_offline(C)
                                        memcg_reparent_objcgs(C)
                                        mem_cgroup_private_id_kill(C)
                                      /* M2 gets C's kmemcg_id and
                                       * allocates lru->xa[idx] */
  memcg_list_lru_allocated(C, lru)
    xa_load(&lru->xa, idx)  /* M2's mlru */
    /* returns true; P is not allocated */
  rcu_read_unlock()

deferred_split_folio(folio)
  folio_lru_add(folio)
    lock_list_lru_of_memcg(lru, nid, &P)
      list_lru_from_memcg(lru, nid, P)  /* NULL */
      VM_WARN_ON(!css_is_dying(&P->css))
      /* walks up to root and queues folio on root's list */

Once P later allocates its own list_lru_memcg,
__folio_unqueue_deferred_split() locks P's list_lru_one while unlinking the
folio from root's list without holding root's lock, corrupting the list.  A
matching RCU read lock around xa_load() and the mlru->memcg == memcg check
in memcg_list_lru_allocated() would avoid this.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112221248.3512447-1-shakeel.butt@linux.dev?part=1

Re: [PATCH] mm: list_lru: don't use a new memcg's lists for an offline memcg

[Shakeel Butt]

On Fri, Nov 13, 2026 at 12:15:11AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue to consider:
> - [High] mm: list_lru: missing mlru->memcg check in
>   memcg_list_lru_allocated()
> --
> 
> commit d213b82b7b4ca4978c858df10a2a8cdea14626e4
> Author: Shakeel Butt <shakeel.butt@linux.dev>
> 
> mm: list_lru: don't use a new memcg's lists for an offline memcg
> 
> This commit records the owning memcg in struct list_lru_memcg and checks it
> in list_lru_from_memcg() so that an offline memcg whose kmemcg_id was reused
> does not use the new memcg's lists.
> 
> > diff --git a/mm/list_lru.c b/mm/list_lru.c
> > index d1d256822ccc7..937dbf239c9d4 100644
> > --- a/mm/list_lru.c
> > +++ b/mm/list_lru.c
> > @@ -72,12 +72,15 @@ static int lru_shrinker_id(struct list_lru *lru)
> >  }
> >  
> >  static inline struct list_lru_one *
> > -list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
> > +list_lru_from_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg)
> >  {
> > +	int idx = memcg_kmem_id(memcg);
> > +
> >  	if (list_lru_memcg_aware(lru) && idx >= 0) {
> >  		struct list_lru_memcg *mlru = xa_load(&lru->xa, idx);
> >  
> > -		return mlru ? &mlru->node[nid] : NULL;
> > +		/* Lists at a reused index belong to another memcg. */
> > +		return mlru && mlru->memcg == memcg ? &mlru->node[nid] : NULL;
> >  	}
> >  	return &lru->node[nid].lru;
> >  }
> 
> [ ... ]
> 
> > @@ -602,6 +604,8 @@ static int __memcg_list_lru_alloc(struct mem_cgroup *memcg,
> >  			if (!mlru)
> >  				return -ENOMEM;
> >  		}
> > +		/* Set before the lists are published, and never changed after. */
> > +		mlru->memcg = pos;
> >  		xas_set(&xas, pos->kmemcg_id);
> >  		do {
> >  			xas_lock_irqsave(&xas, flags);
> 
> [Severity: High]
> Does memcg_list_lru_allocated() also need to check mlru->memcg == memcg?

Yes this is correct and Andrew already have a fix for this.