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

[Matthew Wilcox <willy@xxxxxxxxxxxxx>]

On Thu, Sep 24, 2020 at 11:00:02AM +0200, peterz@xxxxxxxxxxxxx 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@xxxxxxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxx>

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