* [PATCH] page_alloc: Fix freeing non-compound pages @ 2020-09-22 14:00 Matthew Wilcox (Oracle) 2020-09-22 14:35 ` Matthew Wilcox 2020-09-24 9:00 ` peterz 0 siblings, 2 replies; 4+ messages in thread From: Matthew Wilcox (Oracle) @ 2020-09-22 14:00 UTC (permalink / raw) To: Andrew Morton Cc: Matthew Wilcox (Oracle), Nick Piggin, Hugh Dickins, Peter Zijlstra, Daisuke Nishimura, linux-mm, linux-kernel 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] page_alloc: Fix freeing non-compound pages 2020-09-22 14:00 [PATCH] page_alloc: Fix freeing non-compound pages Matthew Wilcox (Oracle) @ 2020-09-22 14:35 ` Matthew Wilcox 2020-09-24 9:00 ` peterz 1 sibling, 0 replies; 4+ messages in thread From: Matthew Wilcox @ 2020-09-22 14:35 UTC (permalink / raw) To: Andrew Morton Cc: Nick Piggin, Hugh Dickins, Peter Zijlstra, Daisuke Nishimura, linux-mm, linux-kernel 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] page_alloc: Fix freeing non-compound pages 2020-09-22 14:00 [PATCH] page_alloc: Fix freeing non-compound pages Matthew Wilcox (Oracle) 2020-09-22 14:35 ` Matthew Wilcox @ 2020-09-24 9:00 ` peterz 2020-09-24 11:07 ` Matthew Wilcox 1 sibling, 1 reply; 4+ messages in thread From: peterz @ 2020-09-24 9:00 UTC (permalink / raw) To: Matthew Wilcox (Oracle) Cc: Andrew Morton, Hugh Dickins, Daisuke Nishimura, linux-mm, linux-kernel, npiggin 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? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] page_alloc: Fix freeing non-compound pages 2020-09-24 9:00 ` peterz @ 2020-09-24 11:07 ` Matthew Wilcox 0 siblings, 0 replies; 4+ messages in thread From: Matthew Wilcox @ 2020-09-24 11:07 UTC (permalink / raw) To: peterz Cc: Andrew Morton, Hugh Dickins, Daisuke Nishimura, linux-mm, linux-kernel, npiggin 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); ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-09-24 11:07 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-22 14:00 [PATCH] page_alloc: Fix freeing non-compound pages Matthew Wilcox (Oracle) 2020-09-22 14:35 ` Matthew Wilcox 2020-09-24 9:00 ` peterz 2020-09-24 11:07 ` Matthew Wilcox
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.