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 Signed-off-by: Shakeel Butt --- 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