2024-04-15 06:31:29

by Anshuman Khandual

Subject: [PATCH] arm64/hugetlb: Fix page table walk in huge_pte_alloc()

Currently normal HugeTLB fault ends up crashing the kernel, as p4dp derived
from p4d_offset() is an invalid address when PGTABLE_LEVEL = 5. A p4d level
entry needs to be allocated when not available while walking the page table
during HugeTLB faults. Let's call p4d_alloc() to allocate such entries when
required instead of current p4d_offset().

Unable to handle kernel paging request at virtual address ffffffff80000000
Mem abort info:
ESR = 0x0000000096000005
EC = 0x25: DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
FSC = 0x05: level 1 translation fault
Data abort info:
ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
CM = 0, WnR = 0, TnD = 0, TagAccess = 0
GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
swapper pgtable: 4k pages, 52-bit VAs, pgdp=0000000081da9000
[ffffffff80000000] pgd=1000000082cec003, p4d=0000000082c32003, pud=0000000000000000
Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP
Modules linked in:
CPU: 1 PID: 108 Comm: high_addr_hugep Not tainted 6.9.0-rc4 #48
Hardware name: Foundation-v8A (DT)
pstate: 01402005 (nzcv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
pc : huge_pte_alloc+0xd4/0x334
lr : hugetlb_fault+0x1b8/0xc68
sp : ffff8000833bbc20
x29: ffff8000833bbc20 x28: fff000080080cb58 x27: ffff800082a7cc58
x26: 0000000000000000 x25: fff0000800378e40 x24: fff00008008d6c60
x23: 00000000de9dbf07 x22: fff0000800378e40 x21: 0004000000000000
x20: 0004000000000000 x19: ffffffff80000000 x18: 1ffe00010011d7a1
x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000001
x14: 0000000000000000 x13: ffff8000816120d0 x12: ffffffffffffffff
x11: 0000000000000000 x10: fff00008008ebd0c x9 : 0004000000000000
x8 : 0000000000001255 x7 : fff00008003e2000 x6 : 00000000061d54b0
x5 : 0000000000001000 x4 : ffffffff80000000 x3 : 0000000000200000
x2 : 0000000000000004 x1 : 0000000080000000 x0 : 0000000000000000
Call trace:
huge_pte_alloc+0xd4/0x334
hugetlb_fault+0x1b8/0xc68
handle_mm_fault+0x260/0x29c
do_page_fault+0xfc/0x47c
do_translation_fault+0x68/0x74
do_mem_abort+0x44/0x94
el0_da+0x2c/0x9c
el0t_64_sync_handler+0x70/0xc4
el0t_64_sync+0x190/0x194
Code: aa000084 cb010084 b24c2c84 8b130c93 (f9400260)
---[ end trace 0000000000000000 ]---

Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Ard Biesheuvel <[email protected]>
Cc: Ryan Roberts <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: [email protected]
Cc: [email protected]
Fixes: a6bbf5d4d9d1 ("arm64: mm: Add definitions to support 5 levels of paging")
Reported-by: Dev Jain <[email protected]>
Signed-off-by: Anshuman Khandual <[email protected]>
---
This patch applies on v6.9-rc4

arch/arm64/mm/hugetlbpage.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
index 0f0e10bb0a95..5c819459555a 100644
--- a/arch/arm64/mm/hugetlbpage.c
+++ b/arch/arm64/mm/hugetlbpage.c
@@ -276,7 +276,7 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
pte_t *ptep = NULL;

pgdp = pgd_offset(mm, addr);
- p4dp = p4d_offset(pgdp, addr);
+ p4dp = p4d_alloc(mm, pgdp, addr);
pudp = pud_alloc(mm, p4dp, addr);
if (!pudp)
return NULL;
--
2.25.1

2024-04-15 06:39:59

by Ard Biesheuvel

Subject: Re: [PATCH] arm64/hugetlb: Fix page table walk in huge_pte_alloc()

Hello Anshuman,

On Mon, 15 Apr 2024 at 08:31, Anshuman Khandual
<[email protected]> wrote:
>
> Currently normal HugeTLB fault ends up crashing the kernel, as p4dp derived
> from p4d_offset() is an invalid address when PGTABLE_LEVEL = 5. A p4d level
> entry needs to be allocated when not available while walking the page table
> during HugeTLB faults. Let's call p4d_alloc() to allocate such entries when
> required instead of current p4d_offset().
>
> Unable to handle kernel paging request at virtual address ffffffff80000000
> Mem abort info:
> ESR = 0x0000000096000005
> EC = 0x25: DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> FSC = 0x05: level 1 translation fault
> Data abort info:
> ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
> CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> swapper pgtable: 4k pages, 52-bit VAs, pgdp=0000000081da9000
> [ffffffff80000000] pgd=1000000082cec003, p4d=0000000082c32003, pud=0000000000000000
> Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 1 PID: 108 Comm: high_addr_hugep Not tainted 6.9.0-rc4 #48
> Hardware name: Foundation-v8A (DT)
> pstate: 01402005 (nzcv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> pc : huge_pte_alloc+0xd4/0x334
> lr : hugetlb_fault+0x1b8/0xc68
> sp : ffff8000833bbc20
> x29: ffff8000833bbc20 x28: fff000080080cb58 x27: ffff800082a7cc58
> x26: 0000000000000000 x25: fff0000800378e40 x24: fff00008008d6c60
> x23: 00000000de9dbf07 x22: fff0000800378e40 x21: 0004000000000000
> x20: 0004000000000000 x19: ffffffff80000000 x18: 1ffe00010011d7a1
> x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000001
> x14: 0000000000000000 x13: ffff8000816120d0 x12: ffffffffffffffff
> x11: 0000000000000000 x10: fff00008008ebd0c x9 : 0004000000000000
> x8 : 0000000000001255 x7 : fff00008003e2000 x6 : 00000000061d54b0
> x5 : 0000000000001000 x4 : ffffffff80000000 x3 : 0000000000200000
> x2 : 0000000000000004 x1 : 0000000080000000 x0 : 0000000000000000
> Call trace:
> huge_pte_alloc+0xd4/0x334
> hugetlb_fault+0x1b8/0xc68
> handle_mm_fault+0x260/0x29c
> do_page_fault+0xfc/0x47c
> do_translation_fault+0x68/0x74
> do_mem_abort+0x44/0x94
> el0_da+0x2c/0x9c
> el0t_64_sync_handler+0x70/0xc4
> el0t_64_sync+0x190/0x194
> Code: aa000084 cb010084 b24c2c84 8b130c93 (f9400260)
> ---[ end trace 0000000000000000 ]---
>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
> Cc: Ard Biesheuvel <[email protected]>
> Cc: Ryan Roberts <[email protected]>
> Cc: Mark Rutland <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Fixes: a6bbf5d4d9d1 ("arm64: mm: Add definitions to support 5 levels of paging")
> Reported-by: Dev Jain <[email protected]>
> Signed-off-by: Anshuman Khandual <[email protected]>
> ---
> This patch applies on v6.9-rc4
>
> arch/arm64/mm/hugetlbpage.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>

Acked-by: Ard Biesheuvel <[email protected]>

Thanks for fixing this. One question below.


> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> index 0f0e10bb0a95..5c819459555a 100644
> --- a/arch/arm64/mm/hugetlbpage.c
> +++ b/arch/arm64/mm/hugetlbpage.c
> @@ -276,7 +276,7 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
> pte_t *ptep = NULL;
>
> pgdp = pgd_offset(mm, addr);
> - p4dp = p4d_offset(pgdp, addr);
> + p4dp = p4d_alloc(mm, pgdp, addr);

Shouldn't we check p4dp for NULL here?

> pudp = pud_alloc(mm, p4dp, addr);
> if (!pudp)
> return NULL;
> --
> 2.25.1
>

2024-04-15 06:53:05

by Anshuman Khandual

Subject: Re: [PATCH] arm64/hugetlb: Fix page table walk in huge_pte_alloc()

On 4/15/24 12:08, Ard Biesheuvel wrote:
> Hello Anshuman,
>
> On Mon, 15 Apr 2024 at 08:31, Anshuman Khandual
> <[email protected]> wrote:
>>
>> Currently normal HugeTLB fault ends up crashing the kernel, as p4dp derived
>> from p4d_offset() is an invalid address when PGTABLE_LEVEL = 5. A p4d level
>> entry needs to be allocated when not available while walking the page table
>> during HugeTLB faults. Let's call p4d_alloc() to allocate such entries when
>> required instead of current p4d_offset().
>>
>> Unable to handle kernel paging request at virtual address ffffffff80000000
>> Mem abort info:
>> ESR = 0x0000000096000005
>> EC = 0x25: DABT (current EL), IL = 32 bits
>> SET = 0, FnV = 0
>> EA = 0, S1PTW = 0
>> FSC = 0x05: level 1 translation fault
>> Data abort info:
>> ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
>> CM = 0, WnR = 0, TnD = 0, TagAccess = 0
>> GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
>> swapper pgtable: 4k pages, 52-bit VAs, pgdp=0000000081da9000
>> [ffffffff80000000] pgd=1000000082cec003, p4d=0000000082c32003, pud=0000000000000000
>> Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP
>> Modules linked in:
>> CPU: 1 PID: 108 Comm: high_addr_hugep Not tainted 6.9.0-rc4 #48
>> Hardware name: Foundation-v8A (DT)
>> pstate: 01402005 (nzcv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
>> pc : huge_pte_alloc+0xd4/0x334
>> lr : hugetlb_fault+0x1b8/0xc68
>> sp : ffff8000833bbc20
>> x29: ffff8000833bbc20 x28: fff000080080cb58 x27: ffff800082a7cc58
>> x26: 0000000000000000 x25: fff0000800378e40 x24: fff00008008d6c60
>> x23: 00000000de9dbf07 x22: fff0000800378e40 x21: 0004000000000000
>> x20: 0004000000000000 x19: ffffffff80000000 x18: 1ffe00010011d7a1
>> x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000001
>> x14: 0000000000000000 x13: ffff8000816120d0 x12: ffffffffffffffff
>> x11: 0000000000000000 x10: fff00008008ebd0c x9 : 0004000000000000
>> x8 : 0000000000001255 x7 : fff00008003e2000 x6 : 00000000061d54b0
>> x5 : 0000000000001000 x4 : ffffffff80000000 x3 : 0000000000200000
>> x2 : 0000000000000004 x1 : 0000000080000000 x0 : 0000000000000000
>> Call trace:
>> huge_pte_alloc+0xd4/0x334
>> hugetlb_fault+0x1b8/0xc68
>> handle_mm_fault+0x260/0x29c
>> do_page_fault+0xfc/0x47c
>> do_translation_fault+0x68/0x74
>> do_mem_abort+0x44/0x94
>> el0_da+0x2c/0x9c
>> el0t_64_sync_handler+0x70/0xc4
>> el0t_64_sync+0x190/0x194
>> Code: aa000084 cb010084 b24c2c84 8b130c93 (f9400260)
>> ---[ end trace 0000000000000000 ]---
>>
>> Cc: Catalin Marinas <[email protected]>
>> Cc: Will Deacon <[email protected]>
>> Cc: Ard Biesheuvel <[email protected]>
>> Cc: Ryan Roberts <[email protected]>
>> Cc: Mark Rutland <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Fixes: a6bbf5d4d9d1 ("arm64: mm: Add definitions to support 5 levels of paging")
>> Reported-by: Dev Jain <[email protected]>
>> Signed-off-by: Anshuman Khandual <[email protected]>
>> ---
>> This patch applies on v6.9-rc4
>>
>> arch/arm64/mm/hugetlbpage.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>
> Acked-by: Ard Biesheuvel <[email protected]>
>
> Thanks for fixing this. One question below.
>
>
>> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
>> index 0f0e10bb0a95..5c819459555a 100644
>> --- a/arch/arm64/mm/hugetlbpage.c
>> +++ b/arch/arm64/mm/hugetlbpage.c
>> @@ -276,7 +276,7 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
>> pte_t *ptep = NULL;
>>
>> pgdp = pgd_offset(mm, addr);
>> - p4dp = p4d_offset(pgdp, addr);
>> + p4dp = p4d_alloc(mm, pgdp, addr);
>
> Shouldn't we check p4dp for NULL here?

Sure, makes sense. I will add a check for p4dp, just like pudp below.

>
>> pudp = pud_alloc(mm, p4dp, addr);
>> if (!pudp)
>> return NULL;
>> --
>> 2.25.1
>>

2024-04-15 08:38:10

by Mark Rutland

Subject: Re: [PATCH] arm64/hugetlb: Fix page table walk in huge_pte_alloc()

On Mon, Apr 15, 2024 at 12:22:40PM +0530, Anshuman Khandual wrote:
>
>
> On 4/15/24 12:08, Ard Biesheuvel wrote:
> > Hello Anshuman,
> >
> > On Mon, 15 Apr 2024 at 08:31, Anshuman Khandual
> > <[email protected]> wrote:
> >>
> >> Currently normal HugeTLB fault ends up crashing the kernel, as p4dp derived
> >> from p4d_offset() is an invalid address when PGTABLE_LEVEL = 5. A p4d level
> >> entry needs to be allocated when not available while walking the page table
> >> during HugeTLB faults. Let's call p4d_alloc() to allocate such entries when
> >> required instead of current p4d_offset().
> >>
> >> Unable to handle kernel paging request at virtual address ffffffff80000000
> >> Mem abort info:
> >> ESR = 0x0000000096000005
> >> EC = 0x25: DABT (current EL), IL = 32 bits
> >> SET = 0, FnV = 0
> >> EA = 0, S1PTW = 0
> >> FSC = 0x05: level 1 translation fault
> >> Data abort info:
> >> ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000
> >> CM = 0, WnR = 0, TnD = 0, TagAccess = 0
> >> GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> >> swapper pgtable: 4k pages, 52-bit VAs, pgdp=0000000081da9000
> >> [ffffffff80000000] pgd=1000000082cec003, p4d=0000000082c32003, pud=0000000000000000
> >> Internal error: Oops: 0000000096000005 [#1] PREEMPT SMP
> >> Modules linked in:
> >> CPU: 1 PID: 108 Comm: high_addr_hugep Not tainted 6.9.0-rc4 #48
> >> Hardware name: Foundation-v8A (DT)
> >> pstate: 01402005 (nzcv daif +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> >> pc : huge_pte_alloc+0xd4/0x334
> >> lr : hugetlb_fault+0x1b8/0xc68
> >> sp : ffff8000833bbc20
> >> x29: ffff8000833bbc20 x28: fff000080080cb58 x27: ffff800082a7cc58
> >> x26: 0000000000000000 x25: fff0000800378e40 x24: fff00008008d6c60
> >> x23: 00000000de9dbf07 x22: fff0000800378e40 x21: 0004000000000000
> >> x20: 0004000000000000 x19: ffffffff80000000 x18: 1ffe00010011d7a1
> >> x17: 0000000000000001 x16: ffffffffffffffff x15: 0000000000000001
> >> x14: 0000000000000000 x13: ffff8000816120d0 x12: ffffffffffffffff
> >> x11: 0000000000000000 x10: fff00008008ebd0c x9 : 0004000000000000
> >> x8 : 0000000000001255 x7 : fff00008003e2000 x6 : 00000000061d54b0
> >> x5 : 0000000000001000 x4 : ffffffff80000000 x3 : 0000000000200000
> >> x2 : 0000000000000004 x1 : 0000000080000000 x0 : 0000000000000000
> >> Call trace:
> >> huge_pte_alloc+0xd4/0x334
> >> hugetlb_fault+0x1b8/0xc68
> >> handle_mm_fault+0x260/0x29c
> >> do_page_fault+0xfc/0x47c
> >> do_translation_fault+0x68/0x74
> >> do_mem_abort+0x44/0x94
> >> el0_da+0x2c/0x9c
> >> el0t_64_sync_handler+0x70/0xc4
> >> el0t_64_sync+0x190/0x194
> >> Code: aa000084 cb010084 b24c2c84 8b130c93 (f9400260)
> >> ---[ end trace 0000000000000000 ]---
> >>
> >> Cc: Catalin Marinas <[email protected]>
> >> Cc: Will Deacon <[email protected]>
> >> Cc: Ard Biesheuvel <[email protected]>
> >> Cc: Ryan Roberts <[email protected]>
> >> Cc: Mark Rutland <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Fixes: a6bbf5d4d9d1 ("arm64: mm: Add definitions to support 5 levels of paging")
> >> Reported-by: Dev Jain <[email protected]>
> >> Signed-off-by: Anshuman Khandual <[email protected]>
> >> ---
> >> This patch applies on v6.9-rc4
> >>
> >> arch/arm64/mm/hugetlbpage.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >
> > Acked-by: Ard Biesheuvel <[email protected]>
> >
> > Thanks for fixing this. One question below.
> >
> >
> >> diff --git a/arch/arm64/mm/hugetlbpage.c b/arch/arm64/mm/hugetlbpage.c
> >> index 0f0e10bb0a95..5c819459555a 100644
> >> --- a/arch/arm64/mm/hugetlbpage.c
> >> +++ b/arch/arm64/mm/hugetlbpage.c
> >> @@ -276,7 +276,7 @@ pte_t *huge_pte_alloc(struct mm_struct *mm, struct vm_area_struct *vma,
> >> pte_t *ptep = NULL;
> >>
> >> pgdp = pgd_offset(mm, addr);
> >> - p4dp = p4d_offset(pgdp, addr);
> >> + p4dp = p4d_alloc(mm, pgdp, addr);
> >
> > Shouldn't we check p4dp for NULL here?
>
> Sure, makes sense. I will add a check for p4dp, just like pudp below.

With that:

Acked-by: Mark Rutland <[email protected]>

Mark.

>
> >
> >> pudp = pud_alloc(mm, p4dp, addr);
> >> if (!pudp)
> >> return NULL;
> >> --
> >> 2.25.1
> >>