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

[sashiko-bot@kernel.org]

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