Skip to contentSkip to conversation

xip: fix get_zeroed_page with __GFP_HIGHMEM

PATCH In review Akinobu Mita 3 participants 3 messages Dec 25, 2007 – Jan 7, 2008

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 xip: fix get_zeroed_page with __GFP_HIGHMEM

The use of get_zeroed_page() with __GFP_HIGHMEM is invalid. Use alloc_page() with __GFP_ZERO instead of invalid get_zeroed_page().

(This patch is only compile tested)

---
 mm/filemap_xip.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
mm/filemap_xip.c +5 -4
Index: 2.6-git/mm/filemap_xip.c
===================================================================
@@ -25,14 +25,15 @@ static struct page *__xip_sparse_page;
 static struct page *xip_sparse_page(void)
 {
 	if (!__xip_sparse_page) {
-		unsigned long zeroes = get_zeroed_page(GFP_HIGHUSER);
-		if (zeroes) {
+		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_ZERO);
+
+		if (page) {
 			static DEFINE_SPINLOCK(xip_alloc_lock);
 			spin_lock(&xip_alloc_lock);
 			if (!__xip_sparse_page)
-				__xip_sparse_page = virt_to_page(zeroes);
+				__xip_sparse_page = page;
 			else
-				free_page(zeroes);
+				__free_page(page);
 			spin_unlock(&xip_alloc_lock);
 		}
 	}
Acked-by on v1 1/1
#1 lore raw
Quoted 7 lines · Akinobu Mita

The use of get_zeroed_page() with __GFP_HIGHMEM is invalid. Use alloc_page() with __GFP_ZERO instead of invalid get_zeroed_page().

(This patch is only compile tested)

Good find! You got me very worried, how this escaped testing before. Presumed explanation: it hasn't been needed beyond s390, which has no CONFIG_HIGHMEM; and it has never been tested with CONFIG_DEBUG_VM on.

But I haven't tested it either: let's wait for Carsten to report. I believe Nick has changes on the way which will make it possible for ordinary mortals to test XIP: here's a good argument to bring them on.

May I cross-reference my "prep_zero_page: remove bogus BUG_ON" 09f345da758fca1222b0971b65b2fddbdf78bb83 in 2.6.24-rc: that bogus (actually VM_)BUG_ON would have stood in the way too, so there's no point in backporting this without that. But if only non-HIGHMEM architectures can have been using XIP, a backport is not essential.

Hugh

p.s. Nick's ZERO_PAGE changes, in 2.6.24-rc, actually cancel the need for a special xip_sparse_page distinct from ZERO_PAGE. But let's not become dependent on those: keep this doing it the way it does now.

Quoted 29 lines
---
 mm/filemap_xip.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: 2.6-git/mm/filemap_xip.c
===================================================================

mm/filemap_xip.c +5 -4
@@ -25,14 +25,15 @@ static struct page *__xip_sparse_page;
 static struct page *xip_sparse_page(void)
 {
 	if (!__xip_sparse_page) {
-		unsigned long zeroes = get_zeroed_page(GFP_HIGHUSER);
-		if (zeroes) {
+		struct page *page = alloc_page(GFP_HIGHUSER | __GFP_ZERO);
+
+		if (page) {
 			static DEFINE_SPINLOCK(xip_alloc_lock);
 			spin_lock(&xip_alloc_lock);
 			if (!__xip_sparse_page)
-				__xip_sparse_page = virt_to_page(zeroes);
+				__xip_sparse_page = page;
 			else
-				free_page(zeroes);
+				__free_page(page);
 			spin_unlock(&xip_alloc_lock);
 		}
 	}