Andrew, Paulus, please apply
The powerpc specific version of hugetlb_get_unmapped_area() makes some
unwarranted assumptions about what checks have been made to its
parameters by its callers. This will lead to a BUG_ON() if a 32-bit
process attempts to make a hugepage mapping which extends above
TASK_SIZE (4GB).
I'm not sure if these assumptions came about because they were valid
with earlier versions of the get_unmapped_area() path, or if it was
always broken. Nonetheless this patch fixes the logic, and removes
the crash.
Signed-off-by: David Gibson <[email protected]>
Index: working-2.6/arch/powerpc/mm/hugetlbpage.c
===================================================================
--- working-2.6.orig/arch/powerpc/mm/hugetlbpage.c 2006-12-21 14:54:15.000000000 +1100
+++ working-2.6/arch/powerpc/mm/hugetlbpage.c 2006-12-21 14:57:35.000000000 +1100
@@ -744,7 +744,8 @@ static int htlb_check_hinted_area(unsign
struct vm_area_struct *vma;
vma = find_vma(current->mm, addr);
- if (!vma || ((addr + len) <= vma->vm_start))
+ if (TASK_SIZE - len >= addr &&
+ (!vma || ((addr + len) <= vma->vm_start)))
return 0;
return -ENOMEM;
@@ -815,6 +816,8 @@ unsigned long hugetlb_get_unmapped_area(
return -EINVAL;
if (len & ~HPAGE_MASK)
return -EINVAL;
+ if (len > TASK_SIZE)
+ return -ENOMEM;
if (!cpu_has_feature(CPU_FTR_16M_PAGE))
return -EINVAL;
@@ -823,9 +826,6 @@ unsigned long hugetlb_get_unmapped_area(
BUG_ON((addr + len) < addr);
if (test_thread_flag(TIF_32BIT)) {
- /* Paranoia, caller should have dealt with this */
- BUG_ON((addr + len) > 0x100000000UL);
-
curareas = current->mm->context.low_htlb_areas;
/* First see if we can use the hint address */
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
> + if (len > TASK_SIZE)
> + return -ENOMEM;
Shouldn't that be addr+len instead? The check looks incomplete
otherwise. And you meant ">=" I guess?
> - /* Paranoia, caller should have dealt with this */
> - BUG_ON((addr + len) > 0x100000000UL);
> -
Any real reason to remove the paranoia check? If it's trivially
always satisfied, the compiler will get rid of it for you :-)
Cheers,
Segher
On Fri, Dec 22, 2006 at 01:31:26AM +0100, Segher Boessenkool wrote:
> > + if (len > TASK_SIZE)
> > + return -ENOMEM;
>
> Shouldn't that be addr+len instead? The check looks incomplete
> otherwise. And you meant ">=" I guess?
No. Have a look at the other hugetlb_get_unmapped_area()
implementations. Because this is in the get_unmapped_area() path,
'addr' is just a hint, so checking addr+len would give bogus
failures. This test is, I believe, essentially an optimization - if
it fails, we're never going to find a suitable addr, so we might as
well give up now.
> > - /* Paranoia, caller should have dealt with this */
> > - BUG_ON((addr + len) > 0x100000000UL);
> > -
>
> Any real reason to remove the paranoia check? If it's trivially
> always satisfied, the compiler will get rid of it for you :-)
Yes - this is the very bug on which was causing crashes - the "caller
should have dealt with this" comment is wrong. The test has been
moved into htlb_check_hinted_area() and now simply fails (and so falls
back to searching for a suitable address), rather than BUG()ing.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
>>> + if (len > TASK_SIZE)
>>> + return -ENOMEM;
>>
>> Shouldn't that be addr+len instead? The check looks incomplete
>> otherwise. And you meant ">=" I guess?
>
> No. Have a look at the other hugetlb_get_unmapped_area()
> implementations. Because this is in the get_unmapped_area() path,
> 'addr' is just a hint,
Ah I missed this vital piece of information, thanks for the
explanation. Care putting in a code comment pointing this out?
> so checking addr+len would give bogus
> failures. This test is, I believe, essentially an optimization - if
> it fails, we're never going to find a suitable addr, so we might as
> well give up now.
Yes, it all makes sense now.
>>> - /* Paranoia, caller should have dealt with this */
>>> - BUG_ON((addr + len) > 0x100000000UL);
>>> -
>>
>> Any real reason to remove the paranoia check? If it's trivially
>> always satisfied, the compiler will get rid of it for you :-)
>
> Yes - this is the very bug on which was causing crashes - the "caller
> should have dealt with this" comment is wrong. The test has been
> moved into htlb_check_hinted_area() and now simply fails (and so falls
> back to searching for a suitable address), rather than BUG()ing.
Yep.
Cheers,
Segher