diff mbox

arm64: Fix THP protection change logic

Message ID 1443701167-10816-1-git-send-email-steve.capper@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Steve Capper Oct. 1, 2015, 12:06 p.m. UTC 
6910fa1 ("arm64: enable PTE type bit in the mask for pte_modify") fixes
a problem whereby a large block of PROT_NONE mapped memory is
incorrectly mapped as block descriptors when mprotect is called.

Unfortunately, a subtle bug was introduced by this fix to the THP logic.

If one mmaps a large block of memory, then faults it such that it is
collapsed into THPs; resulting calls to mprotect on this area of memory
will lead to incorrect table descriptors being written instead of block
descriptors. This is because pmd_modify calls pte_modify which is now
allowed to modify the type of the page table entry.

This patch reverts commit 6910fa16dbe142f6a0fd0fd7c249f9883ff7fc8a, and
fixes the problem it was trying to address by adjusting PAGE_NONE to
represent a table entry. Thus no change in pte type is required when
moving from PROT_NONE to a different protection.

Fixes: 6910fa16dbe1 ("arm64: enable PTE type bit in the mask for pte_modify")
Cc: Feng Kan <fkan@apm.com>
Reported-by: Ganapatrao Kulkarni <Ganapatrao.Kulkarni@caviumnetworks.com>
Signed-off-by: Steve Capper <steve.capper@linaro.org>
---

This patch stops my THP mprotect tests from crashing my machine quite
horrendously.

Ganapatrao, could you please give this a test and let me know if it behaves
with your NUMA stuff?

Feng, does this revised fix work with the Trinity test you mentioned in
6910fa1 ("arm64: enable PTE type bit in the mask for pte_modify")?

Cheers,

Comments

Catalin Marinas Oct. 1, 2015, 2:07 p.m. UTC | #1 
On Thu, Oct 01, 2015 at 01:06:07PM +0100, Steve Capper wrote:
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index 56283f8..cf73194 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -80,7 +80,7 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
>  #define PAGE_S2			__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY)
>  #define PAGE_S2_DEVICE		__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_UXN)
>  
> -#define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_TYPE_MASK) | PTE_PROT_NONE | PTE_PXN | PTE_UXN)
> +#define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_PXN | PTE_UXN)
>  #define PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
>  #define PAGE_SHARED_EXEC	__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_WRITE)
>  #define PAGE_COPY		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN)
> @@ -460,7 +460,7 @@ static inline pud_t *pud_offset(pgd_t *pgd, unsigned long addr)
>  static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
>  {
>  	const pteval_t mask = PTE_USER | PTE_PXN | PTE_UXN | PTE_RDONLY |
> -			      PTE_PROT_NONE | PTE_WRITE | PTE_TYPE_MASK;
> +			      PTE_PROT_NONE | PTE_VALID | PTE_WRITE;
>  	pte_val(pte) = (pte_val(pte) & ~mask) | (pgprot_val(newprot) & mask);
>  	return pte;
>  }

Thanks for looking into this. So for ptes, we need the type bits to be:

0b?0: unmapped/PAGE_NONE
0b11: mapped

With pmd block (huge page), we want:

0b?0: unmapped/PAGE_NONE
0b01: mapped

With your fix, bit 1 is no longer touched by pte_modify(). It looks fine
to me. Another option would have been a separate pmd_modify() function
with its own mask but I think it's cleaner this way.

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Let's see if it fixes the issue for Ganapatrao and Will or I will send
it upstream.
Ganapatrao Kulkarni Oct. 1, 2015, 4:09 p.m. UTC | #2 
Thanks Steve.
this patch fixes the issue i was facing when running applications
(which forms thp) with auto numa enabled.

Tested-by: Ganapatrao Kulkarni <gkulkarni@caviumnetworks.com>

On Thu, Oct 1, 2015 at 7:37 PM, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Thu, Oct 01, 2015 at 01:06:07PM +0100, Steve Capper wrote:
>> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
>> index 56283f8..cf73194 100644
>> --- a/arch/arm64/include/asm/pgtable.h
>> +++ b/arch/arm64/include/asm/pgtable.h
>> @@ -80,7 +80,7 @@ extern void __pgd_error(const char *file, int line, unsigned long val);
>>  #define PAGE_S2                      __pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY)
>>  #define PAGE_S2_DEVICE               __pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_UXN)
>>
>> -#define PAGE_NONE            __pgprot(((_PAGE_DEFAULT) & ~PTE_TYPE_MASK) | PTE_PROT_NONE | PTE_PXN | PTE_UXN)
>> +#define PAGE_NONE            __pgprot(((_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_PXN | PTE_UXN)
>>  #define PAGE_SHARED          __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
>>  #define PAGE_SHARED_EXEC     __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_WRITE)
>>  #define PAGE_COPY            __pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN)
>> @@ -460,7 +460,7 @@ static inline pud_t *pud_offset(pgd_t *pgd, unsigned long addr)
>>  static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
>>  {
>>       const pteval_t mask = PTE_USER | PTE_PXN | PTE_UXN | PTE_RDONLY |
>> -                           PTE_PROT_NONE | PTE_WRITE | PTE_TYPE_MASK;
>> +                           PTE_PROT_NONE | PTE_VALID | PTE_WRITE;
>>       pte_val(pte) = (pte_val(pte) & ~mask) | (pgprot_val(newprot) & mask);
>>       return pte;
>>  }
>
> Thanks for looking into this. So for ptes, we need the type bits to be:
>
> 0b?0: unmapped/PAGE_NONE
> 0b11: mapped
>
> With pmd block (huge page), we want:
>
> 0b?0: unmapped/PAGE_NONE
> 0b01: mapped
>
> With your fix, bit 1 is no longer touched by pte_modify(). It looks fine
> to me. Another option would have been a separate pmd_modify() function
> with its own mask but I think it's cleaner this way.
>
> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
>
> Let's see if it fixes the issue for Ganapatrao and Will or I will send
> it upstream.
>
> --
> Catalin
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

thanks
Ganapat
Steve Capper Oct. 1, 2015, 4:21 p.m. UTC | #3 
On 1 October 2015 at 17:09, Ganapatrao Kulkarni <gpkulkarni@gmail.com> wrote:
> Thanks Steve.
> this patch fixes the issue i was facing when running applications
> (which forms thp) with auto numa enabled.
>
> Tested-by: Ganapatrao Kulkarni <gkulkarni@caviumnetworks.com>
>

Brilliant, thank you Ganapat.

Catalin, thanks for the review, would you like me to resend with these
two tags added, or are you happy to apply them before merging?

Cheers,
--
Steve
Catalin Marinas Oct. 1, 2015, 5:02 p.m. UTC | #4 
On Thu, Oct 01, 2015 at 05:21:02PM +0100, Steve Capper wrote:
> On 1 October 2015 at 17:09, Ganapatrao Kulkarni <gpkulkarni@gmail.com> wrote:
> > Thanks Steve.
> > this patch fixes the issue i was facing when running applications
> > (which forms thp) with auto numa enabled.
> >
> > Tested-by: Ganapatrao Kulkarni <gkulkarni@caviumnetworks.com>
> >
> 
> Brilliant, thank you Ganapat.
> 
> Catalin, thanks for the review, would you like me to resend with these
> two tags added, or are you happy to apply them before merging?

No need to resend, I'll add the tags and send a pull request tomorrow.

Thanks.
diff mbox

Patch

diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
index 56283f8..cf73194 100644
--- a/arch/arm64/include/asm/pgtable.h
+++ b/arch/arm64/include/asm/pgtable.h
@@ -80,7 +80,7 @@  extern void __pgd_error(const char *file, int line, unsigned long val);
 #define PAGE_S2			__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_NORMAL) | PTE_S2_RDONLY)
 #define PAGE_S2_DEVICE		__pgprot(PROT_DEFAULT | PTE_S2_MEMATTR(MT_S2_DEVICE_nGnRE) | PTE_S2_RDONLY | PTE_UXN)
 
-#define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_TYPE_MASK) | PTE_PROT_NONE | PTE_PXN | PTE_UXN)
+#define PAGE_NONE		__pgprot(((_PAGE_DEFAULT) & ~PTE_VALID) | PTE_PROT_NONE | PTE_PXN | PTE_UXN)
 #define PAGE_SHARED		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN | PTE_WRITE)
 #define PAGE_SHARED_EXEC	__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_WRITE)
 #define PAGE_COPY		__pgprot(_PAGE_DEFAULT | PTE_USER | PTE_NG | PTE_PXN | PTE_UXN)
@@ -460,7 +460,7 @@  static inline pud_t *pud_offset(pgd_t *pgd, unsigned long addr)
 static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
 {
 	const pteval_t mask = PTE_USER | PTE_PXN | PTE_UXN | PTE_RDONLY |
-			      PTE_PROT_NONE | PTE_WRITE | PTE_TYPE_MASK;
+			      PTE_PROT_NONE | PTE_VALID | PTE_WRITE;
 	pte_val(pte) = (pte_val(pte) & ~mask) | (pgprot_val(newprot) & mask);
 	return pte;
 }