Re: Rare memory leakage

[Matthew Wilcox <willy@infradead.org>]

On Tue, Sep 22, 2020 at 01:22:12PM +0200, Vlastimil Babka wrote:
> On 9/22/20 5:12 AM, Matthew Wilcox wrote:
> > This is a fun little race.
> > 
> > Dramatis personae: Pages P0, P1 are consecutive and aligned.
> > Threads A, B, C.
> > 
> > Page P0 is allocated to the page cache.
> > Page P1 is free.
> > 
> > Thread A calls find_get_entry()
> > P0 is returned from xas_load()
> > 
> > Thread B removes the page from the page cache (eg truncate, invalidatepage).
> > P0 is buddy-merged with P1.
> > 
> > Thread C calls alloc_pages, order 1, does not specify GFP_COMP.  P0 now
> > has refcount 1.
> > 
> > Thread A calls page_cache_get_speculative().  P0 has refcount 2.
> > 
> > Thread C calls __free_page(P0, 1)
> > put_page_testzero is _false_.  Do not call free_the_page().
> > 
> > Thread A calls put_page(P0)
> > We free P0 and nobody knows to free P1.
> > 
> > 
> > Weird solution: In __free_page(), if put_page_testzero() fails and page
> > is not PageHead, convert it to a compound page.  Then the put_page()
> > by Thread A will free P1.
> 
> I can imagine doing the conversion in a manner that deals with races properly
> will be rather tricky...

Yes.  We can't just look at the return from put_page_testzero() because
by then thread A may have put its ref too and the page is already winding
its way through the page freeing mechanism.  We could try to conditionally
get the reference back again, but we're then looking at the same race.

I've just kicked off a test run using this:

+/*
+ * 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);


> > Better ideas?
> 
> IMHO, alloc_pages() with order > 0 and without __GFP_COMP is a weird beast. In
> that case it would be probably best to refcount each base page separately. I
> don't know how many assumptions this would break :/

You sound like a man who's never dealt with device drivers.  Multiorder,
non-compound allocations are the norm in that world.