Thread (13 messages) 13 messages, 4 authors, 2021-03-26

Re: [PATCH] mm: page_alloc: fix memcg accounting leak in speculative cache lookup

From: Michal Hocko <mhocko@suse.com>
Date: 2021-03-24 08:58:57
Also in: linux-mm, lkml

On Tue 23-03-21 15:02:32, Johannes Weiner wrote:
[...]
quoted
From f6f062a3ec46f4fb083dcf6792fde9723f18cfc5 Mon Sep 17 00:00:00 2001
From: Johannes Weiner <hannes@cmpxchg.org>
Date: Fri, 19 Mar 2021 02:17:00 -0400
Subject: [PATCH] mm: page_alloc: fix allocation imbalances from speculative
 cache lookup

When the freeing of a higher-order page block (non-compound) races
with a speculative page cache lookup, __free_pages() needs to leave
the first order-0 page in the chunk to the lookup but free the buddy
pages that the lookup doesn't know about separately.

There are currently two problems with it:

1. It checks PageHead() to see whether we're dealing with a compound
   page after put_page_testzero(). But the speculative lookup could
   have freed the page after our put and cleared PageHead, in which
   case we would double free the tail pages.

   To fix this, test PageHead before the put and cache the result for
   afterwards.

2. If such a higher-order page is charged to a memcg (e.g. !vmap
   kernel stack)), only the first page of the block has page->memcg
   set. That means we'll uncharge only one order-0 page from the
   entire block, and leak the remainder.

   To fix this, add a split_page_memcg() before it starts freeing tail
   pages, to ensure they all have page->memcg set up.

While at it, also update the comments a bit to clarify what exactly is
happening to the page during that race.

Fixes: e320d3012d25 mm/page_alloc.c: fix freeing non-compound pages
Reported-by: Hugh Dickins <hughd@google.com>
Reported-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Cc: <redacted> # 5.10+
Normally I would argue for one fix per patch but considering we need
both anyway then squashing is fine. Especially with the above
explanation. Thanks for updated comments it made it much clear what the
speculative path is doing. I was quite confused about the whole thing
earlier.

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!
quoted hunk ↗ jump to hunk
---
 mm/page_alloc.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c53fe4fa10bf..8aab1e87fa3c 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5112,10 +5112,9 @@ static inline void free_the_page(struct page *page, unsigned int order)
  * the allocation, so it is easy to leak memory.  Freeing more memory
  * than was allocated will probably emit a warning.
  *
- * If the last reference to this page is speculative, it will be released
- * by put_page() which only frees the first page of a non-compound
- * allocation.  To prevent the remaining pages from being leaked, we free
- * the subsequent pages here.  If you want to use the page's reference
+ * This function isn't a put_page(). Don't let the put_page_testzero()
+ * fool you, it's only to deal with speculative cache references. It
+ * WILL free pages directly. If you want to use the page's reference
  * count to decide when to free the allocation, you should allocate a
  * compound page, and use put_page() instead of __free_pages().
  *
@@ -5124,11 +5123,41 @@ static inline void free_the_page(struct page *page, unsigned int order)
  */
 void __free_pages(struct page *page, unsigned int order)
 {
-	if (put_page_testzero(page))
+	bool compound = PageHead(page);
+
+	/*
+	 * Drop the base reference from __alloc_pages and free. In
+	 * case there is an outstanding speculative reference, from
+	 * e.g. the page cache, it will put and free the page later.
+	 */
+	if (likely(put_page_testzero(page))) {
 		free_the_page(page, order);
-	else if (!PageHead(page))
+		return;
+	}
+
+	/*
+	 * Ok, the speculative reference will put and free the page.
+	 *
+	 * - If this was an order-0 page, we're done.
+	 *
+	 * - If the page was compound, the other side will free the
+	 *   entire page and we're done here as well. Just note that
+	 *   freeing clears PG_head, so it can only be read reliably
+	 *   before the put_page_testzero().
+	 *
+	 * - If the page was of higher order but NOT marked compound,
+	 *   the other side will know nothing about our buddy pages
+	 *   and only free the order-0 page at the start of our block.
+	 *   We must split off and free the buddy pages here.
+	 *
+	 *   The buddy pages aren't individually refcounted, so they
+	 *   can't have any pending speculative references themselves.
+	 */
+	if (order > 0 && !compound) {
+		split_page_memcg(page, 1 << order);
 		while (order-- > 0)
 			free_the_page(page + (1 << order), order);
+	}
 }
 EXPORT_SYMBOL(__free_pages);
 
-- 
2.31.0
-- 
Michal Hocko
SUSE Labs
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help