mm: add gfp mask checking for __get_free_pages()

PATCH In review Akinobu Mita 2 participants 3 messages Jul 4, 2009 – Jul 18, 2009

Subsystems (2)

Memory management Page allocator

lore

Share & tools

lore

Akinobu Mita

author

2009-07-04 02:12 UTC #0 lore raw

PATCH 1/1 mm: add gfp mask checking for __get_free_pages()

__get_free_pages() with __GFP_HIGHMEM is not safe because the return address cannot represent a highmem page. get_zeroed_page() already has such a debug checking.

Signed-off-by Akinobu Mita <[email protected]>

---
 mm/page_alloc.c |   24 +++++++++---------------
 1 files changed, 9 insertions(+), 15 deletions(-)

mm/page_alloc.c +9 ~~-15~~

index e0f2cdf..4a1a374 100644
@@ -1903,31 +1903,25 @@ EXPORT_SYMBOL(__alloc_pages_nodemask);
  */
 unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
 {
-	struct page * page;
+	struct page *page;
+
+	/*
+	 * __get_free_pages() returns a 32-bit address, which cannot represent
+	 * a highmem page
+	 */
+	VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
+
 	page = alloc_pages(gfp_mask, order);
 	if (!page)
 		return 0;
 	return (unsigned long) page_address(page);
 }
-
 EXPORT_SYMBOL(__get_free_pages);
 
 unsigned long get_zeroed_page(gfp_t gfp_mask)
 {
-	struct page * page;
-
-	/*
-	 * get_zeroed_page() returns a 32-bit address, which cannot represent
-	 * a highmem page
-	 */
-	VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
-
-	page = alloc_pages(gfp_mask | __GFP_ZERO, 0);
-	if (page)
-		return (unsigned long) page_address(page);
-	return 0;
+	return __get_free_pages(gfp_mask | __GFP_ZERO, 0);
 }
-
 EXPORT_SYMBOL(get_zeroed_page);
 
 void __pagevec_free(struct pagevec *pvec)

Signature · 1 line

1.6.0.6

Andrew Morton

maintainer

2009-07-18 00:41 UTC #1 lore raw

Quoted 44 lines · On Sat, 4 Jul 2009 11:09:50 +0900 Akinobu Mita <[email protected]> wrote:

__get_free_pages() with __GFP_HIGHMEM is not safe because the return address cannot represent a highmem page. get_zeroed_page() already has such a debug checking.

Signed-off-by Akinobu Mita <[email protected]>

---
 mm/page_alloc.c |   24 +++++++++---------------
 1 files changed, 9 insertions(+), 15 deletions(-)

mm/page_alloc.c +8 ~~-10~~

index e0f2cdf..4a1a374 100644
@@ -1903,31 +1903,25 @@ EXPORT_SYMBOL(__alloc_pages_nodemask);
  */
 unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
 {
-	struct page * page;
+	struct page *page;
+
+	/*
+	 * __get_free_pages() returns a 32-bit address, which cannot represent
+	 * a highmem page
+	 */
+	VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
+
 	page = alloc_pages(gfp_mask, order);
 	if (!page)
 		return 0;
 	return (unsigned long) page_address(page);
 }
-
 EXPORT_SYMBOL(__get_free_pages);
 
 unsigned long get_zeroed_page(gfp_t gfp_mask)
 {
-	struct page * page;
-
-	/*
-	 * get_zeroed_page() returns a 32-bit address, which cannot represent
-	 * a highmem page
-	 */
-	VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
-

mm/page_alloc.c EXPORT_SYMBOL(__alloc_pages_nodemask); · line 1927

Quoted 10 lines

mm/page_alloc.c +1 -5

-	page = alloc_pages(gfp_mask | __GFP_ZERO, 0);
-	if (page)
-		return (unsigned long) page_address(page);
-	return 0;
+	return __get_free_pages(gfp_mask | __GFP_ZERO, 0);
 }
-
 EXPORT_SYMBOL(get_zeroed_page);
 
 void __pagevec_free(struct pagevec *pvec)

Fair enough.

I suspect we could just delete that VM_BUG_ON() - we can't go and do runtime checking for every darn programmer error, and this would be a pretty dumb one.

Your patch turns get_zeroed_page() into a simple one-liner wrapper around __get_free_pages(). We could perhaps save some .text, some kernel stack and some CPU cycles by inlining get_zeroed_page().

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Akinobu Mita

author

2009-07-18 01:29 UTC #2 lore raw

2009/7/18 Andrew Morton <[email protected]>:

Quoted 56 lines

Quoted 53 lines · On Sat, 4 Jul 2009 11:09:50 +0900 Akinobu Mita <[email protected]> wrote:

__get_free_pages() with __GFP_HIGHMEM is not safe because the return address cannot represent a highmem page. get_zeroed_page() already has such a debug checking.

Signed-off-by Akinobu Mita <[email protected]>

---
mm/page_alloc.c | 24 +++++++++---------------

 1 files changed, 9 insertions(+), 15 deletions(-)

mm/page_alloc.c +9 ~~-15~~

index e0f2cdf..4a1a374 100644
@@ -1903,31 +1903,25 @@ EXPORT_SYMBOL(__alloc_pages_nodemask);
   */
  unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
  {
-     struct page * page;
+     struct page *page;
+
+     /*
+      * __get_free_pages() returns a 32-bit address, which cannot represent
+      * a highmem page
+      */
+     VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
+
       page = alloc_pages(gfp_mask, order);
       if (!page)
               return 0;
       return (unsigned long) page_address(page);
  }
-
  EXPORT_SYMBOL(__get_free_pages);
 
  unsigned long get_zeroed_page(gfp_t gfp_mask)
  {
-     struct page * page;
-
-     /*
-      * get_zeroed_page() returns a 32-bit address, which cannot represent
-      * a highmem page
-      */
-     VM_BUG_ON((gfp_mask & __GFP_HIGHMEM) != 0);
-
-     page = alloc_pages(gfp_mask | __GFP_ZERO, 0);
-     if (page)
-             return (unsigned long) page_address(page);
-     return 0;
+     return __get_free_pages(gfp_mask | __GFP_ZERO, 0);
  }
-
  EXPORT_SYMBOL(get_zeroed_page);

Quoted 7 lines

mm/page_alloc.c EXPORT_SYMBOL(__alloc_pages_nodemask); · line 1927
Quoted 1 line
mm/page_alloc.c +0 -0
  void __pagevec_free(struct pagevec *pvec)
Fair enough.
I suspect we could just delete that VM_BUG_ON() - we can't go and do runtime checking for every darn programmer error, and this would be a pretty dumb one.

Maybe. But we had such a bug in c51b1a160b63304720d49479986915e4c475a2cf (xip: fix get_zeroed_page with __GFP_HIGHME). Even the VM code had it and did not fixed for a long time.