[PATCH] page_alloc: Fix freeing non-compound pages

[PATCH] page_alloc: Fix freeing non-compound pages

[Matthew Wilcox (Oracle)]

Here is a very rare race which leaks memory:

Page P0 is allocated to the page cache.
Page P1 is free.

Thread A		Thread B		Thread C
find_get_entry():
xas_load() returns P0
						Removes P0 from page cache
						Frees P0
						P0 merged with its buddy P1
			alloc_pages(GFP_KERNEL, 1) returns P0
			P0 has refcount 1
page_cache_get_speculative(P0)
P0 has refcount 2
			__free_pages(P0)
			P0 has refcount 1
put_page(P0)
P1 is not freed

Fix this by freeing all the pages in __free_pages() that won't be freed
by the call to put_page().  It's usually not a good idea to split a page,
but this is a very unlikely scenario.

Fixes: e286781d5f2e ("mm: speculative page references")
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/page_alloc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index fab5e97dc9ca..5db74797db39 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4943,10 +4943,19 @@ static inline void free_the_page(struct page *page, unsigned int order)
 		__free_pages_ok(page, order);
 }
 
+/*
+ * If we free a non-compound allocation, another thread may have a
+ * speculative reference to the first page.  It has no way of knowing
+ * about the rest of the allocation, so we have to free all but the
+ * first page here.
+ */
 void __free_pages(struct page *page, unsigned int order)
 {
 	if (put_page_testzero(page))
 		free_the_page(page, order);
+	else
+		while (order-- > 0)
+			free_the_page(page + (1 << order), order);
 }
 EXPORT_SYMBOL(__free_pages);
 
-- 
2.28.0

Re: [PATCH] page_alloc: Fix freeing non-compound pages

[Matthew Wilcox]

On Tue, Sep 22, 2020 at 03:00:17PM +0100, Matthew Wilcox (Oracle) wrote:
>  void __free_pages(struct page *page, unsigned int order)
>  {
>  	if (put_page_testzero(page))
>  		free_the_page(page, order);
> +	else
> +		while (order-- > 0)
> +			free_the_page(page + (1 << order), order);
>  }
>  EXPORT_SYMBOL(__free_pages);

... a three line patch and one of them is wrong.

-       else
+       else if (!PageHead(page))

Anyone got a smart idea about how to _test_ this code path?  I'm
wondering about loading one kernel module which wanders through memmap
calling
	if (page_cache_get_speculative(page)) put_page(page);
and another kernel module that calls
	__free_pages(alloc_page(GFP_KERNEL, 1), 1);

and putting in a printk to let me know when we hit it.

Re: [PATCH] page_alloc: Fix freeing non-compound pages

[peterz]

On Tue, Sep 22, 2020 at 03:00:17PM +0100, Matthew Wilcox (Oracle) wrote:
> Here is a very rare race which leaks memory:
> 
> Page P0 is allocated to the page cache.
> Page P1 is free.
> 
> Thread A		Thread B		Thread C
> find_get_entry():
> xas_load() returns P0
> 						Removes P0 from page cache
> 						Frees P0
> 						P0 merged with its buddy P1
> 			alloc_pages(GFP_KERNEL, 1) returns P0
> 			P0 has refcount 1
> page_cache_get_speculative(P0)
> P0 has refcount 2
> 			__free_pages(P0)
> 			P0 has refcount 1
> put_page(P0)
> P1 is not freed
> 
> Fix this by freeing all the pages in __free_pages() that won't be freed
> by the call to put_page().  It's usually not a good idea to split a page,
> but this is a very unlikely scenario.
> 
> Fixes: e286781d5f2e ("mm: speculative page references")
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  mm/page_alloc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index fab5e97dc9ca..5db74797db39 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4943,10 +4943,19 @@ static inline void free_the_page(struct page *page, unsigned int order)
>  		__free_pages_ok(page, order);
>  }
>  
> +/*
> + * If we free a non-compound allocation, another thread may have a
> + * speculative reference to the first page.  It has no way of knowing
> + * about the rest of the allocation, so we have to free all but the
> + * first page here.
> + */
>  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);
>  }
>  EXPORT_SYMBOL(__free_pages);

So the obvious question I have here is why not teach put_page() to free
the whole thing?

Re: [PATCH] page_alloc: Fix freeing non-compound pages

[Matthew Wilcox]

On Thu, Sep 24, 2020 at 11:00:02AM +0200, peterz@infradead.org wrote:
> On Tue, Sep 22, 2020 at 03:00:17PM +0100, Matthew Wilcox (Oracle) wrote:
> > Here is a very rare race which leaks memory:
> > 
> > Page P0 is allocated to the page cache.
> > Page P1 is free.
> > 
> > Thread A		Thread B		Thread C
> > find_get_entry():
> > xas_load() returns P0
> > 						Removes P0 from page cache
> > 						Frees P0
> > 						P0 merged with its buddy P1
> > 			alloc_pages(GFP_KERNEL, 1) returns P0
> > 			P0 has refcount 1
> > page_cache_get_speculative(P0)
> > P0 has refcount 2
> > 			__free_pages(P0)
> > 			P0 has refcount 1
> > put_page(P0)
> > P1 is not freed
> > 
> > Fix this by freeing all the pages in __free_pages() that won't be freed
> > by the call to put_page().  It's usually not a good idea to split a page,
> > but this is a very unlikely scenario.
> > 
> > Fixes: e286781d5f2e ("mm: speculative page references")
> > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > ---
> >  mm/page_alloc.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index fab5e97dc9ca..5db74797db39 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -4943,10 +4943,19 @@ static inline void free_the_page(struct page *page, unsigned int order)
> >  		__free_pages_ok(page, order);
> >  }
> >  
> > +/*
> > + * If we free a non-compound allocation, another thread may have a
> > + * speculative reference to the first page.  It has no way of knowing
> > + * about the rest of the allocation, so we have to free all but the
> > + * first page here.
> > + */
> >  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);
> >  }
> >  EXPORT_SYMBOL(__free_pages);
> 
> So the obvious question I have here is why not teach put_page() to free
> the whole thing?

That's more complicated.  It looks like this:

    Fix this by converting P0 into a compound page if it is not freed by
    __free_pages().
    
    Fixes: e286781d5f2e ("mm: speculative page references")
    Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index fab5e97dc9ca..3e9f6e6694e7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -4943,10 +4943,25 @@ static inline void free_the_page(struct page *page, unsigned int order)
                __free_pages_ok(page, order);
 }
 
+/*
+ * Have to be careful when freeing a non-compound allocation in case somebody
+ * else takes a temporary reference on the first page and then calls put_page()
+ */
 void __free_pages(struct page *page, unsigned int order)
 {
-       if (put_page_testzero(page))
-               free_the_page(page, order);
+       if (likely(page_ref_freeze(page, 1)))
+               goto free;
+       if (likely(order == 0 || PageHead(page))) {
+               if (put_page_testzero(page))
+                       goto free;
+               return;
+       }
+
+       prep_compound_page(page, order);
+       put_page(page);
+       return;
+free:
+       free_the_page(page, order);
 }
 EXPORT_SYMBOL(__free_pages);