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

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

[Johannes Weiner]

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.

However, 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.

Add a split_page_memcg() to __free_pages() right before it starts
taking the higher-order page apart and freeing its individual
constituent pages. This ensures all of them will have the memcg
linkage set up for correct uncharging. Also update the comments a bit
to clarify what exactly is happening to the page during that race.

This bug is old and has its roots in the speculative page cache patch
and adding cgroup accounting of kernel pages. There are no known user
reports. A backport to stable is therefor not warranted.

Reported-by: Matthew Wilcox <willy@infradead.org>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/page_alloc.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c53fe4fa10bf..f4bd56656402 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,33 @@ 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))
+	/*
+	 * 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;
+	}
+
+	/*
+	 * The speculative reference will put and free the page.
+	 *
+	 * However, if the speculation was into a higher-order page
+	 * chunk that isn't marked compound, the other side will know
+	 * nothing about our buddy pages and only free the order-0
+	 * page at the start of our chunk! 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 (!PageHead(page) && order > 0) {
+		split_page_memcg(page, 1 << order);
 		while (order-- > 0)
 			free_the_page(page + (1 << order), order);
+	}
 }
 EXPORT_SYMBOL(__free_pages);
 
-- 
2.30.1

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

[Matthew Wilcox]

On Fri, Mar 19, 2021 at 03:15:47AM -0400, Johannes Weiner wrote:
> 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.

Thanks.

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

[Hugh Dickins]

On Fri, 19 Mar 2021, Johannes Weiner wrote:

> 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.
> 
> However, 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.
> 
> Add a split_page_memcg() to __free_pages() right before it starts
> taking the higher-order page apart and freeing its individual
> constituent pages. This ensures all of them will have the memcg
> linkage set up for correct uncharging. Also update the comments a bit
> to clarify what exactly is happening to the page during that race.
> 
> This bug is old and has its roots in the speculative page cache patch
> and adding cgroup accounting of kernel pages. There are no known user
> reports. A backport to stable is therefor not warranted.
> 
> Reported-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

Acked-by: Hugh Dickins <hughd@google.com>

to the split_page_memcg() addition etc, but a doubt just hit me on the
original e320d3012d25 ("mm/page_alloc.c: fix freeing non-compound pages"):
see comment below.

> ---
>  mm/page_alloc.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c53fe4fa10bf..f4bd56656402 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,33 @@ 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))
> +	/*
> +	 * 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;
> +	}
> +
> +	/*
> +	 * The speculative reference will put and free the page.
> +	 *
> +	 * However, if the speculation was into a higher-order page
> +	 * chunk that isn't marked compound, the other side will know
> +	 * nothing about our buddy pages and only free the order-0
> +	 * page at the start of our chunk! 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 (!PageHead(page) && order > 0) {

The put_page_testzero() has released our reference to the first
subpage of page: it's now under the control of the racing speculative
lookup.  So it seems to me unsafe to be checking PageHead(page) here:
if it was actually a compound page, PageHead might already be cleared
by now, and we doubly free its tail pages below?  I think we need to
use a "bool compound = PageHead(page)" on entry to __free_pages().

Or alternatively, it's wrong to call __free_pages() on a compound
page anyway, so we should not check PageHead at all, except in a
WARN_ON_ONCE(PageCompound(page)) at the start?

And would it be wrong to fix that too in this patch?
Though it ought then to be backported to 5.10 stable.

> +		split_page_memcg(page, 1 << order);
>  		while (order-- > 0)
>  			free_the_page(page + (1 << order), order);
> +	}
>  }
>  EXPORT_SYMBOL(__free_pages);
>  
> -- 
> 2.30.1

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

[Matthew Wilcox]

On Fri, Mar 19, 2021 at 06:52:58PM -0700, Hugh Dickins wrote:
> > +	/*
> > +	 * 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;
> > +	}
> > +
> > +	/*
> > +	 * The speculative reference will put and free the page.
> > +	 *
> > +	 * However, if the speculation was into a higher-order page
> > +	 * chunk that isn't marked compound, the other side will know
> > +	 * nothing about our buddy pages and only free the order-0
> > +	 * page at the start of our chunk! 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 (!PageHead(page) && order > 0) {
> 
> The put_page_testzero() has released our reference to the first
> subpage of page: it's now under the control of the racing speculative
> lookup.  So it seems to me unsafe to be checking PageHead(page) here:
> if it was actually a compound page, PageHead might already be cleared
> by now, and we doubly free its tail pages below?  I think we need to
> use a "bool compound = PageHead(page)" on entry to __free_pages().
> 
> Or alternatively, it's wrong to call __free_pages() on a compound
> page anyway, so we should not check PageHead at all, except in a
> WARN_ON_ONCE(PageCompound(page)) at the start?

Alas ...

$ git grep '__free_pages\>.*compound'
drivers/dma-buf/heaps/system_heap.c:            __free_pages(page, compound_order(page));
drivers/dma-buf/heaps/system_heap.c:            __free_pages(p, compound_order(p));
drivers/dma-buf/heaps/system_heap.c:            __free_pages(page, compound_order(page));
mm/huge_memory.c:               __free_pages(zero_page, compound_order(zero_page));
mm/huge_memory.c:               __free_pages(zero_page, compound_order(zero_page));
mm/slub.c:                      __free_pages(page, compound_order(page));

Maybe we should disallow it!

There are a few other places to check:

$ grep -l __GFP_COMP $(git grep -lw __free_pages) | wc -l
24

(assuming the pages are allocated and freed in the same file, which is a
reasonable approximation, but not guaranteed to catch everything.  Many
of these 24 will be false positives, of course.)

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

[Johannes Weiner]

On Fri, Mar 19, 2021 at 06:52:58PM -0700, Hugh Dickins wrote:
> On Fri, 19 Mar 2021, Johannes Weiner wrote:
> 
> > 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.
> > 
> > However, 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.
> > 
> > Add a split_page_memcg() to __free_pages() right before it starts
> > taking the higher-order page apart and freeing its individual
> > constituent pages. This ensures all of them will have the memcg
> > linkage set up for correct uncharging. Also update the comments a bit
> > to clarify what exactly is happening to the page during that race.
> > 
> > This bug is old and has its roots in the speculative page cache patch
> > and adding cgroup accounting of kernel pages. There are no known user
> > reports. A backport to stable is therefor not warranted.
> > 
> > Reported-by: Matthew Wilcox <willy@infradead.org>
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> Acked-by: Hugh Dickins <hughd@google.com>
> 
> to the split_page_memcg() addition etc, but a doubt just hit me on the
> original e320d3012d25 ("mm/page_alloc.c: fix freeing non-compound pages"):
> see comment below.
> 
> > ---
> >  mm/page_alloc.c | 33 +++++++++++++++++++++++++++------
> >  1 file changed, 27 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index c53fe4fa10bf..f4bd56656402 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,33 @@ 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))
> > +	/*
> > +	 * 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;
> > +	}
> > +
> > +	/*
> > +	 * The speculative reference will put and free the page.
> > +	 *
> > +	 * However, if the speculation was into a higher-order page
> > +	 * chunk that isn't marked compound, the other side will know
> > +	 * nothing about our buddy pages and only free the order-0
> > +	 * page at the start of our chunk! 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 (!PageHead(page) && order > 0) {
> 
> The put_page_testzero() has released our reference to the first
> subpage of page: it's now under the control of the racing speculative
> lookup.  So it seems to me unsafe to be checking PageHead(page) here:
> if it was actually a compound page, PageHead might already be cleared
> by now, and we doubly free its tail pages below?  I think we need to
> use a "bool compound = PageHead(page)" on entry to __free_pages().

That's a good point.

> And would it be wrong to fix that too in this patch?

All aboard the mm-page_alloc-fix-stuff.patch!

No, I think it's fine to sqash them and treat it as a supplement to
Matthew's original patch (although technically it didn't make the
memcg leak any worse).

> Though it ought then to be backported to 5.10 stable.

Sounds good. It depends on split_page_memcg(), but that patch is
straight-forward enough to backport as well.

---

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: <stable@vger.kernel.org> # 5.10+
---
 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

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

[Matthew Wilcox]

On Tue, Mar 23, 2021 at 03:02:32PM -0400, Johannes Weiner wrote:
> >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: <stable@vger.kernel.org> # 5.10+

This version makes me happy.

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Thanks for fixing my buggy fix.

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

[Hugh Dickins]

On Tue, 23 Mar 2021, Johannes Weiner wrote:
> 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: <stable@vger.kernel.org> # 5.10+

This is great, thanks Hannes.
Acked-by: Hugh Dickins <hughd@google.com>

I know that 5.10-stable rejected the two split_page_memcg() patches:
we shall need those in, I'll send GregKH the fixups, but not today.

> ---
>  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

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

[Hugh Dickins]

On Tue, 23 Mar 2021, Hugh Dickins wrote:
> On Tue, 23 Mar 2021, Johannes Weiner wrote:
> > 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

Whoops, misses ("...") around the title.

> > Reported-by: Hugh Dickins <hughd@google.com>
> > Reported-by: Matthew Wilcox <willy@infradead.org>
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> > Cc: <stable@vger.kernel.org> # 5.10+
> 
> This is great, thanks Hannes.
> Acked-by: Hugh Dickins <hughd@google.com>

Sorry, I am ashamed to do this, but now I renege and say NAK:
better now than before Andrew picks it up.

The first reason occurred to me this morning.  I thought I had been
clever to spot the PageHead race which you fix here.  But now I just feel
very stupid not to have spotted the very similar memcg_data race.  The
speculative racer may call mem_cgroup_uncharge() from __put_single_page(),
and the new call to split_page_memcg() do nothing because page_memcg(head)
is already NULL.

And is it even safe there, to sprinkle memcg_data through all of those
order-0 subpages, when free_the_page() is about to be applied to a
series of descending orders?  I could easily be wrong, but I think
free_pages_prepare()'s check_free_page() will find that is not
page_expected_state().

And what gets to do the uncharging when memcg_data is properly set
on the appropriate order-N subpages?  I believe it's the (second)
__memcg_kmem_uncharge_page() in free_pages_prepare(), but that's
only called if PageMemcgKmem().  Ah, good, Roman's changes have put
that flag into memcg_data, so it will automatically be set: but this
patch will not port back to 5.10 without some addition.

But, after all that, I'm now thinking that Matthew's original
e320d3012d25 ("mm/page_alloc.c: fix freeing non-compound pages")
is safer reverted.  The put_page_testzero() in __free_pages() was
not introduced for speculative pagecache: it was there in 2.4.0,
and atomic_dec_and_test() in 2.2, I don't have older trees to hand.

So, it has "always" been accepted that multiple references to a
high-order non-compound page can be given out and released: maybe
they were all released with __free_pages() of the right order, or
maybe only the last had to get that right; but as __free_pages()
stands today, all but the last caller frees all but the first
subpage.  A very rare leak seems much safer.

I don't have the answer (find somewhere in struct page to squirrel
away the order, even when it's a non-compound page?), and I think
each of us would much rather be thinking about other things at the
moment.  But for now it looks to me like NAK to this patch, and
revert of e320d3012d25.

> 
> I know that 5.10-stable rejected the two split_page_memcg() patches:
> we shall need those in, I'll send GregKH the fixups, but not today.

Done, and Sasha has picked them up.  But in writing that "Ah, good,
Roman's changes ..." paragraph, I've begun to wonder if what I sent
was complete - does a 5.10 split_page_memcg(), when called from
split_page(), also need to copy head's PageKmemcg? I rather think
yes, but by now I'm unsure of everything...

Hugh

> 
> > ---
> >  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

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

[Matthew Wilcox]

On Thu, Mar 25, 2021 at 06:55:42PM -0700, Hugh Dickins wrote:
> The first reason occurred to me this morning.  I thought I had been
> clever to spot the PageHead race which you fix here.  But now I just feel
> very stupid not to have spotted the very similar memcg_data race.  The
> speculative racer may call mem_cgroup_uncharge() from __put_single_page(),
> and the new call to split_page_memcg() do nothing because page_memcg(head)
> is already NULL.
> 
> And is it even safe there, to sprinkle memcg_data through all of those
> order-0 subpages, when free_the_page() is about to be applied to a
> series of descending orders?  I could easily be wrong, but I think
> free_pages_prepare()'s check_free_page() will find that is not
> page_expected_state().

So back to something more like my original patch then?

+++ b/mm/page_alloc.c
@@ -5081,9 +5081,15 @@ void __free_pages(struct page *page, unsigned int order)
 {
        if (put_page_testzero(page))
                free_the_page(page, order);
-	else if (!PageHead(page))
-               while (order-- > 0)
-                       free_the_page(page + (1 << order), order);
+       else if (!PageHead(page)) {
+               while (order-- > 0) {
+                       struct page *tail = page + (1 << order);
+#ifdef CONFIG_MEMCG
+                       tail->memcg_data = page->memcg_data;
+#endif
+                       free_the_page(tail, order);
+               }
+       }
 }
 EXPORT_SYMBOL(__free_pages);

We can cache page->memcg_data before calling put_page_testzero(),
just like we cache the Head flag in Johannes' patch.

> But, after all that, I'm now thinking that Matthew's original
> e320d3012d25 ("mm/page_alloc.c: fix freeing non-compound pages")
> is safer reverted.  The put_page_testzero() in __free_pages() was
> not introduced for speculative pagecache: it was there in 2.4.0,
> and atomic_dec_and_test() in 2.2, I don't have older trees to hand.

I think you're confused in that last assertion.  According to
linux-fullhistory, the first introduction of __free_pages was 2.3.29pre3
(September 1999), where it did indeed use put_page_testzero:

+extern inline void __free_pages(struct page *page, unsigned long order)
+{
+       if (!put_page_testzero(page))
+               return;
+       __free_pages_ok(page, order);
+}

Before that, we had only free_pages() and __free_page().

> So, it has "always" been accepted that multiple references to a
> high-order non-compound page can be given out and released: maybe
> they were all released with __free_pages() of the right order, or
> maybe only the last had to get that right; but as __free_pages()
> stands today, all but the last caller frees all but the first
> subpage.  A very rare leak seems much safer.
> 
> I don't have the answer (find somewhere in struct page to squirrel
> away the order, even when it's a non-compound page?), and I think
> each of us would much rather be thinking about other things at the
> moment.  But for now it looks to me like NAK to this patch, and
> revert of e320d3012d25.

We did discuss that possibility prior to the introduction of
e320d3012d25.  Here's one such:
https://lore.kernel.org/linux-mm/20200922031215.GZ32101@casper.infradead.org/T/#m0b08c0c3430e09e20fa6648877dc42b04b18e6f3

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

[Hugh Dickins]

On Fri, 26 Mar 2021, Matthew Wilcox wrote:
> On Thu, Mar 25, 2021 at 06:55:42PM -0700, Hugh Dickins wrote:
> > The first reason occurred to me this morning.  I thought I had been
> > clever to spot the PageHead race which you fix here.  But now I just feel
> > very stupid not to have spotted the very similar memcg_data race.  The
> > speculative racer may call mem_cgroup_uncharge() from __put_single_page(),
> > and the new call to split_page_memcg() do nothing because page_memcg(head)
> > is already NULL.
> > 
> > And is it even safe there, to sprinkle memcg_data through all of those
> > order-0 subpages, when free_the_page() is about to be applied to a
> > series of descending orders?  I could easily be wrong, but I think
> > free_pages_prepare()'s check_free_page() will find that is not
> > page_expected_state().
> 
> So back to something more like my original patch then?
> 
> +++ b/mm/page_alloc.c
> @@ -5081,9 +5081,15 @@ void __free_pages(struct page *page, unsigned int order)
>  {
>         if (put_page_testzero(page))
>                 free_the_page(page, order);
> -	else if (!PageHead(page))
> -               while (order-- > 0)
> -                       free_the_page(page + (1 << order), order);
> +       else if (!PageHead(page)) {
> +               while (order-- > 0) {
> +                       struct page *tail = page + (1 << order);
> +#ifdef CONFIG_MEMCG
> +                       tail->memcg_data = page->memcg_data;
> +#endif
> +                       free_the_page(tail, order);
> +               }
> +       }
>  }
>  EXPORT_SYMBOL(__free_pages);
> 
> We can cache page->memcg_data before calling put_page_testzero(),
> just like we cache the Head flag in Johannes' patch.

If I still believed in e320d3012d25, yes, that would look right
(but I don't have much faith in my judgement after all this).

I'd fallen in love with split_page_memcg() when you posted that
one, and was put off by your #ifdef, so got my priorities wrong
and went for the split_page_memcg().

> 
> > But, after all that, I'm now thinking that Matthew's original
> > e320d3012d25 ("mm/page_alloc.c: fix freeing non-compound pages")
> > is safer reverted.  The put_page_testzero() in __free_pages() was
> > not introduced for speculative pagecache: it was there in 2.4.0,
> > and atomic_dec_and_test() in 2.2, I don't have older trees to hand.
> 
> I think you're confused in that last assertion.  According to
> linux-fullhistory, the first introduction of __free_pages was 2.3.29pre3
> (September 1999), where it did indeed use put_page_testzero:

Not confused, just pontificating from a misleading subset of the data.
I knew there's an even-more-history-than-tglx git tree somewhere, but
what I usually look back to is 2.4 trees, plus a 2.2.26 tree - but of
course that's a late 2.2, from 2004, around the same time as 2.6.3.
That tree shows a __free_pages() using atomic_dec_and_test().

But we digress...

> 
> +extern inline void __free_pages(struct page *page, unsigned long order)
> +{
> +       if (!put_page_testzero(page))
> +               return;
> +       __free_pages_ok(page, order);
> +}
> 
> Before that, we had only free_pages() and __free_page().
> 
> > So, it has "always" been accepted that multiple references to a
> > high-order non-compound page can be given out and released: maybe
> > they were all released with __free_pages() of the right order, or
> > maybe only the last had to get that right; but as __free_pages()
> > stands today, all but the last caller frees all but the first
> > subpage.  A very rare leak seems much safer.
> > 
> > I don't have the answer (find somewhere in struct page to squirrel
> > away the order, even when it's a non-compound page?), and I think
> > each of us would much rather be thinking about other things at the
> > moment.  But for now it looks to me like NAK to this patch, and
> > revert of e320d3012d25.
> 
> We did discuss that possibility prior to the introduction of
> e320d3012d25.  Here's one such:
> https://lore.kernel.org/linux-mm/20200922031215.GZ32101@casper.infradead.org/T/#m0b08c0c3430e09e20fa6648877dc42b04b18e6f3

Thanks for the link. And I'll willingly grant that your experience is
vast compared to mine. But "Drivers don't do that, in my experience"
is not a convincing reason to invalidate a way of working that the
code has gone out of its way to allow for, for over twenty years.

But you make a good point on the "Bad page" reports that would now
be generated: maybe that will change my mind later on.

Hugh

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

[Matthew Wilcox]

On Thu, Mar 25, 2021 at 09:04:40PM -0700, Hugh Dickins wrote:
> On Fri, 26 Mar 2021, Matthew Wilcox wrote:
> > On Thu, Mar 25, 2021 at 06:55:42PM -0700, Hugh Dickins wrote:
> > > The first reason occurred to me this morning.  I thought I had been
> > > clever to spot the PageHead race which you fix here.  But now I just feel
> > > very stupid not to have spotted the very similar memcg_data race.  The
> > > speculative racer may call mem_cgroup_uncharge() from __put_single_page(),
> > > and the new call to split_page_memcg() do nothing because page_memcg(head)
> > > is already NULL.
> > > 
> > > And is it even safe there, to sprinkle memcg_data through all of those
> > > order-0 subpages, when free_the_page() is about to be applied to a
> > > series of descending orders?  I could easily be wrong, but I think
> > > free_pages_prepare()'s check_free_page() will find that is not
> > > page_expected_state().

I forgot to say earlier; I did add a test (lib/test_free_pages.c).
Doubling it up to check GFP_KERNEL | GFP_ACCOUNT and GFP_KERNEL |
GFP_COMP | GFP_ACCOUNT should be reasonable.

> > So back to something more like my original patch then?
> > 
> > +++ b/mm/page_alloc.c
> > @@ -5081,9 +5081,15 @@ void __free_pages(struct page *page, unsigned int order)
> >  {
> >         if (put_page_testzero(page))
> >                 free_the_page(page, order);
> > -	else if (!PageHead(page))
> > -               while (order-- > 0)
> > -                       free_the_page(page + (1 << order), order);
> > +       else if (!PageHead(page)) {
> > +               while (order-- > 0) {
> > +                       struct page *tail = page + (1 << order);
> > +#ifdef CONFIG_MEMCG
> > +                       tail->memcg_data = page->memcg_data;
> > +#endif
> > +                       free_the_page(tail, order);
> > +               }
> > +       }
> >  }
> >  EXPORT_SYMBOL(__free_pages);
> > 
> > We can cache page->memcg_data before calling put_page_testzero(),
> > just like we cache the Head flag in Johannes' patch.
> 
> If I still believed in e320d3012d25, yes, that would look right
> (but I don't have much faith in my judgement after all this).
> 
> I'd fallen in love with split_page_memcg() when you posted that
> one, and was put off by your #ifdef, so got my priorities wrong
> and went for the split_page_memcg().

Oh, the ifdef was just a strawman.  I wouldn't want to see that upstream;
something like:

	unsigned long memcg_data = __get_memcg_data(page);
...
			__set_memcg_data(tail, memcg_data);

with the appropriate ifdefs hidden in memcontrol.h would be my preference.

> > > But, after all that, I'm now thinking that Matthew's original
> > > e320d3012d25 ("mm/page_alloc.c: fix freeing non-compound pages")
> > > is safer reverted.  The put_page_testzero() in __free_pages() was
> > > not introduced for speculative pagecache: it was there in 2.4.0,
> > > and atomic_dec_and_test() in 2.2, I don't have older trees to hand.
> > 
> > I think you're confused in that last assertion.  According to
> > linux-fullhistory, the first introduction of __free_pages was 2.3.29pre3
> > (September 1999), where it did indeed use put_page_testzero:
> 
> Not confused, just pontificating from a misleading subset of the data.
> I knew there's an even-more-history-than-tglx git tree somewhere, but
> what I usually look back to is 2.4 trees, plus a 2.2.26 tree - but of
> course that's a late 2.2, from 2004, around the same time as 2.6.3.

I suspect it got backported ...
https://github.com/mpe/linux-fullhistory/wiki is what I'm using for my
archaeology, and it doesn't have the stable branches (1.0, 1.2, 2.0,
2.2, 2.4), so I don't know for sure.

Anyway, my point is that the truly ancient drivers *don't* depend on this
behaviour because the function didn't even exist when they were written.

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

[Michal Hocko]

On Tue 23-03-21 15:02:32, Johannes Weiner wrote:
[...]
> >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: <stable@vger.kernel.org> # 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!

> ---
>  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

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

[Michal Hocko]

On Fri 19-03-21 03:15:47, Johannes Weiner wrote:
> 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.
> 
> However, 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.
> 
> Add a split_page_memcg() to __free_pages() right before it starts
> taking the higher-order page apart and freeing its individual
> constituent pages. This ensures all of them will have the memcg
> linkage set up for correct uncharging. Also update the comments a bit
> to clarify what exactly is happening to the page during that race.
> 
> This bug is old and has its roots in the speculative page cache patch
> and adding cgroup accounting of kernel pages. There are no known user
> reports. A backport to stable is therefor not warranted.
> 
> Reported-by: Matthew Wilcox <willy@infradead.org>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>

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

Thanks!

> ---
>  mm/page_alloc.c | 33 +++++++++++++++++++++++++++------
>  1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c53fe4fa10bf..f4bd56656402 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,33 @@ 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))
> +	/*
> +	 * 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;
> +	}
> +
> +	/*
> +	 * The speculative reference will put and free the page.
> +	 *
> +	 * However, if the speculation was into a higher-order page
> +	 * chunk that isn't marked compound, the other side will know
> +	 * nothing about our buddy pages and only free the order-0
> +	 * page at the start of our chunk! 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 (!PageHead(page) && order > 0) {
> +		split_page_memcg(page, 1 << order);
>  		while (order-- > 0)
>  			free_the_page(page + (1 << order), order);
> +	}
>  }
>  EXPORT_SYMBOL(__free_pages);
>  
> -- 
> 2.30.1
> 

-- 
Michal Hocko
SUSE Labs