[PATCH] mmtom : add VM_BUG_ON in __get_free_pages

[PATCH] mmtom : add VM_BUG_ON in __get_free_pages

[MinChan Kim]

The __get_free_pages is used in many place. 
Also, driver developers can use it freely due to export function.
Some developers might use it to allocate high pages by mistake. 

The __get_free_pages can allocate high page using alloc_pages, 
but it can't return linear address for high page.

Even worse, in this csse, caller can't free page which are there in high zone. 
So, It would be better to add VM_BUG_ON. 

It's based on mmtom 2009-02-27-13-54.
 
Signed-off-by: MinChan Kim <minchan.kim@gmail.com>
---
 mm/page_alloc.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 8294107..381056b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1681,6 +1681,13 @@ EXPORT_SYMBOL(__alloc_pages_internal);
 unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
 {
 	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;
-- 
1.5.4.3


-- 
Kinds Regards
MinChan Kim

--
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>

Re: [PATCH] mmtom : add VM_BUG_ON in __get_free_pages

[Andrew Morton]

On Mon, 2 Mar 2009 18:31:48 +0900
MinChan Kim <minchan.kim@gmail.com> wrote:

> 
> The __get_free_pages is used in many place. 
> Also, driver developers can use it freely due to export function.
> Some developers might use it to allocate high pages by mistake. 
> 
> The __get_free_pages can allocate high page using alloc_pages, 
> but it can't return linear address for high page.
> 
> Even worse, in this csse, caller can't free page which are there in high zone. 
> So, It would be better to add VM_BUG_ON. 
> 
> It's based on mmtom 2009-02-27-13-54.
>  
> Signed-off-by: MinChan Kim <minchan.kim@gmail.com>
> ---
>  mm/page_alloc.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 8294107..381056b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1681,6 +1681,13 @@ EXPORT_SYMBOL(__alloc_pages_internal);
>  unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
>  {
>  	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;

If someone calls __get_free_pages(__GFP_HIGHMEM) then page_address()
will reliably return NULL and the caller's code will oops.

Yes, there's a decent (and increasing) risk that the developer won't be
testing the code on a highmem machine, but there are enough highmem
machines out there that the bug should be discovered pretty quickly.

So I'm not sure that this test is worth the additional overhead to a
fairly frequently called function?

--
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>

Re: [PATCH] mmtom : add VM_BUG_ON in __get_free_pages

[Minchan Kim]

On Tue, Mar 3, 2009 at 7:27 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Mon, 2 Mar 2009 18:31:48 +0900
> MinChan Kim <minchan.kim@gmail.com> wrote:
>
>>
>> The __get_free_pages is used in many place.
>> Also, driver developers can use it freely due to export function.
>> Some developers might use it to allocate high pages by mistake.
>>
>> The __get_free_pages can allocate high page using alloc_pages,
>> but it can't return linear address for high page.
>>
>> Even worse, in this csse, caller can't free page which are there in high zone.
>> So, It would be better to add VM_BUG_ON.
>>
>> It's based on mmtom 2009-02-27-13-54.
>>
>> Signed-off-by: MinChan Kim <minchan.kim@gmail.com>
>> ---
>>  mm/page_alloc.c |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 8294107..381056b 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -1681,6 +1681,13 @@ EXPORT_SYMBOL(__alloc_pages_internal);
>>  unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
>>  {
>>       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;
>
> If someone calls __get_free_pages(__GFP_HIGHMEM) then page_address()
> will reliably return NULL and the caller's code will oops.

If someone fail to allocate highmem, he might retry to allocate page
in normal or dma. then he will get the page.
but, more important thing is that In first try, page allocator
succeeded to allocate page in highmem and just failed to translate
from page to linear address by page_address(page).

So, the caller don't have any way to free high page.
How do we solve this problem ?


> Yes, there's a decent (and increasing) risk that the developer won't be
> testing the code on a highmem machine, but there are enough highmem
> machines out there that the bug should be discovered pretty quickly.
> So I'm not sure that this test is worth the additional overhead to a
> fairly frequently called function?
>

I agree.

If you have a concern about hotpath,  how about adding annotation to
prevent wrong usage ?
If you agree with my suggestion, I will resend this patch with more
detail changelog.

 Signed-off-by: MinChan Kim <minchan.kim@gmail.com>

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

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 381056b..b168c5f 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -1677,6 +1677,7 @@ EXPORT_SYMBOL(__alloc_pages_internal);

 /*
  * Common helper functions.
+ * This function must not be used with __GFP_HIGHMEM.
  */
 unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
 {
-- 
1.5.4.3




-- 
Kinds regards,
MinChan Kim

--
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>

Re: [PATCH] mmtom : add VM_BUG_ON in __get_free_pages

[Johannes Weiner]

On Mon, Mar 02, 2009 at 02:27:57PM -0800, Andrew Morton wrote:
> On Mon, 2 Mar 2009 18:31:48 +0900
> MinChan Kim <minchan.kim@gmail.com> wrote:
> 
> > 
> > The __get_free_pages is used in many place. 
> > Also, driver developers can use it freely due to export function.
> > Some developers might use it to allocate high pages by mistake. 
> > 
> > The __get_free_pages can allocate high page using alloc_pages, 
> > but it can't return linear address for high page.
> > 
> > Even worse, in this csse, caller can't free page which are there in high zone. 
> > So, It would be better to add VM_BUG_ON. 
> > 
> > It's based on mmtom 2009-02-27-13-54.
> >  
> > Signed-off-by: MinChan Kim <minchan.kim@gmail.com>
> > ---
> >  mm/page_alloc.c |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 8294107..381056b 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -1681,6 +1681,13 @@ EXPORT_SYMBOL(__alloc_pages_internal);
> >  unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
> >  {
> >  	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;
> 
> If someone calls __get_free_pages(__GFP_HIGHMEM) then page_address()
> will reliably return NULL and the caller's code will oops.

It will allocate a page, fail to translate it to a virtual address,
return 0 and the caller will think allocation failed because it checks
for the return value.

But the highmem page is still allocated and now leaked, isn't it?

> Yes, there's a decent (and increasing) risk that the developer won't be
> testing the code on a highmem machine, but there are enough highmem
> machines out there that the bug should be discovered pretty quickly.

Another thing is that a device driver developer does not necessarily
has CONFIG_DEBUG_VM set.  Can we expect him to?

> So I'm not sure that this test is worth the additional overhead to a
> fairly frequently called function?

Well, it's only done conditionally if you want to debug the thing
anyway.  But as mentioned above, maybe this isn't the right condition.

--
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>

Re: [PATCH] mmtom : add VM_BUG_ON in __get_free_pages

[Minchan Kim]

Hi, Hannes.
Thanks for careful review.

On Tue, Mar 3, 2009 at 8:16 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> On Mon, Mar 02, 2009 at 02:27:57PM -0800, Andrew Morton wrote:
>> On Mon, 2 Mar 2009 18:31:48 +0900
>> MinChan Kim <minchan.kim@gmail.com> wrote:
>>
>> >
>> > The __get_free_pages is used in many place.
>> > Also, driver developers can use it freely due to export function.
>> > Some developers might use it to allocate high pages by mistake.
>> >
>> > The __get_free_pages can allocate high page using alloc_pages,
>> > but it can't return linear address for high page.
>> >
>> > Even worse, in this csse, caller can't free page which are there in high zone.
>> > So, It would be better to add VM_BUG_ON.
>> >
>> > It's based on mmtom 2009-02-27-13-54.
>> >
>> > Signed-off-by: MinChan Kim <minchan.kim@gmail.com>
>> > ---
>> >  mm/page_alloc.c |    7 +++++++
>> >  1 files changed, 7 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> > index 8294107..381056b 100644
>> > --- a/mm/page_alloc.c
>> > +++ b/mm/page_alloc.c
>> > @@ -1681,6 +1681,13 @@ EXPORT_SYMBOL(__alloc_pages_internal);
>> >  unsigned long __get_free_pages(gfp_t gfp_mask, unsigned int order)
>> >  {
>> >     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;
>>
>> If someone calls __get_free_pages(__GFP_HIGHMEM) then page_address()
>> will reliably return NULL and the caller's code will oops.
>
> It will allocate a page, fail to translate it to a virtual address,
> return 0 and the caller will think allocation failed because it checks
> for the return value.
>
> But the highmem page is still allocated and now leaked, isn't it?

That was my point.

>> Yes, there's a decent (and increasing) risk that the developer won't be
>> testing the code on a highmem machine, but there are enough highmem
>> machines out there that the bug should be discovered pretty quickly.
>
> Another thing is that a device driver developer does not necessarily
> has CONFIG_DEBUG_VM set.  Can we expect him to?

Hmm. I agree.
Even, Many embedded guys don't upload their driver source to mainline
for revewing and testing.
so, others who have high mem machine can't review and test driver code.
Of couse, It depends on their willing but it would be better to care them.

>> So I'm not sure that this test is worth the additional overhead to a
>> fairly frequently called function?
>
> Well, it's only done conditionally if you want to debug the thing
> anyway.  But as mentioned above, maybe this isn't the right condition.
>
I added VM_DEUBG_ON as referencing get_zeroed_page without serious considering.
I agree with your opinion.
so, we have to change __get_free_page and get_zeroed_page.
Do you mean BUG_ON instead of VM_BUG_ON ?

Andrew, How about that ?
you think it is rather big overhead even if it's vm debug mode ?

-- 
Kinds regards,
Minchan Kim

--
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>