Re: [PATCH] mm: list_lru: fix NULL deref when a folio moves to the root memcg

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] mm/list_lru: infinite loop in __memcg_list_lru_alloc() on
  xas_nomem() failure [1]
--

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

mm: list_lru: fix NULL deref when a folio moves to the root memcg

This commit skips calling __memcg_list_lru_alloc() in the slow path of
folio_memcg_list_lru_alloc() if the folio's memcg has been reparented to the
root memcg.

> diff --git a/mm/list_lru.c b/mm/list_lru.c
> index d1d256822ccc7..566691ca3dd8f 100644
> --- a/mm/list_lru.c
> +++ b/mm/list_lru.c
> @@ -645,9 +645,15 @@ int folio_memcg_list_lru_alloc(struct folio *folio, struct list_lru *lru,
>  	if (likely(res))
>  		return 0;
>  
> -	/* Allocation may block, pin the memcg */
> +	/*
> +	 * Allocation may block, so pin the memcg. If the folio's cgroup
> +	 * was removed after the check above, the folio has moved up to a
> +	 * parent memcg, which can be the root. The root uses the global
> +	 * lists and has no parent, so skip it.
> +	 */
>  	memcg = get_mem_cgroup_from_folio(folio);
> -	res = __memcg_list_lru_alloc(memcg, lru, gfp);
> +	if (!mem_cgroup_is_root(memcg))
> +		res = __memcg_list_lru_alloc(memcg, lru, gfp);
>  	mem_cgroup_put(memcg);
>  	return res;
>  }

[Severity: High]
This problem wasn't introduced by this patch, but can __memcg_list_lru_alloc()
enter an infinite loop [1] when xas_nomem() fails while populating an
ancestor cgroup?

mm/list_lru.c:__memcg_list_lru_alloc() {
	...
	do {
		pos = memcg;
		parent = parent_mem_cgroup(pos);
		while (!memcg_list_lru_allocated(parent, lru)) {
			pos = parent;
			parent = parent_mem_cgroup(pos);
		}
		...
		xas_set(&xas, pos->kmemcg_id);
		do {
			xas_lock_irqsave(&xas, flags);
			if (!xas_load(&xas) && !css_is_dying(&pos->css)) {
				xas_store(&xas, mlru);
				if (!xas_error(&xas))
					mlru = NULL;
			}
			xas_unlock_irqrestore(&xas, flags);
		} while (xas_nomem(&xas, gfp));
	} while (pos != memcg && !css_is_dying(&pos->css));
	...
	return xas_error(&xas);
}

When storing mlru for an ancestor (pos != memcg) needs a new xa_node,
xas_store() sets xas to -ENOMEM and leaves pos unpopulated in lru->xa.  If
xas_nomem() then fails to allocate a node (for example under memory pressure,
or when gfp &= GFP_RECLAIM_MASK strips GFP_NOWAIT), it returns false with xas
still in the error state.

Because the outer loop does not check !xas_error(&xas), it repeats for the
same unpopulated ancestor pos, where xas_set() resets xas.xa_node to
XAS_RESTART and clears the error, looping forever and causing a soft or hard
lockup.

[1] https://sashiko.dev/#/bug/linux-21c1dba9-8d6f-4368-b38b-9a57cacb056f

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