* Re: [PATCH] mm: add gfp mask checking for __get_free_pages() [not found] <20090704020949.GA3047@localhost.localdomain> @ 2009-07-18 0:41 ` Andrew Morton 2009-07-18 1:29 ` Akinobu Mita 0 siblings, 1 reply; 2+ messages in thread From: Andrew Morton @ 2009-07-18 0:41 UTC (permalink / raw) To: Akinobu Mita; +Cc: linux-kernel, linux-mm On Sat, 4 Jul 2009 11:09:50 +0900 Akinobu Mita <akinobu.mita@gmail.com> 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 <akinobu.mita@gmail.com> > --- > mm/page_alloc.c | 24 +++++++++--------------- > 1 files changed, 9 insertions(+), 15 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index e0f2cdf..4a1a374 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -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) 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 majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] mm: add gfp mask checking for __get_free_pages() 2009-07-18 0:41 ` [PATCH] mm: add gfp mask checking for __get_free_pages() Andrew Morton @ 2009-07-18 1:29 ` Akinobu Mita 0 siblings, 0 replies; 2+ messages in thread From: Akinobu Mita @ 2009-07-18 1:29 UTC (permalink / raw) To: Andrew Morton; +Cc: linux-kernel, linux-mm 2009/7/18 Andrew Morton <akpm@linux-foundation.org>: > On Sat, 4 Jul 2009 11:09:50 +0900 > Akinobu Mita <akinobu.mita@gmail.com> 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 <akinobu.mita@gmail.com> >> --- >> mm/page_alloc.c | 24 +++++++++--------------- >> 1 files changed, 9 insertions(+), 15 deletions(-) >> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index e0f2cdf..4a1a374 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -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) > > 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. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2009-07-18 1:29 UTC | newest] Thread overview: 2+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <20090704020949.GA3047@localhost.localdomain> 2009-07-18 0:41 ` [PATCH] mm: add gfp mask checking for __get_free_pages() Andrew Morton 2009-07-18 1:29 ` Akinobu Mita
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).