[PATCH] Revert "arm64: mm: set the contiguous bit for kernel mappings where appropriate"

[Ard Biesheuvel]

On 23 February 2017 at 16:22, Mark Rutland <mark.rutland at arm.com> wrote:
> This reverts commit 0bfc445dec9dd8130d22c9f4476eed7598524129.
>
> When we change the permissions of regions mapped using contiguous
> entries, the architecture requires us to follow a Break-Before-Make
> strategy, breaking *all* associated entries before we can change any of
> the following properties from the entries:
>
>  - presence of the contiguous bit
>  - output address
>  - attributes
>  - permissiones
>
> Failure to do so can result in a number of problems (e.g. TLB conflict
> aborts and/or erroneous results from TLB lookups).
>
> See ARM DDI 0487A.k_iss10775, "Misprogramming of the Contiguous bit",
> page D4-1762.
>
> We do not take this into account when altering the permissions of kernel
> segments in mark_rodata_ro(), where we change the permissions of live
> contiguous entires one-by-one, leaving them transiently inconsistent.
> This has been observed to result in failures on some fast model
> configurations.
>
> Unfortunately, we cannot follow Break-Before-Make here as we'd have to
> unmap kernel text and data used to perform the sequence.
>
> For the timebeing, revert commit 0bfc445dec9dd813 so as to avoid issues
> resulting from this misuse of the contiguous bit.
>
> Signed-off-by: Mark Rutland <mark.rutland at arm.com>
> Reported-by: Jean-Philippe Brucker <jean-philippe.brucker at arm.com>
> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Cc: Catalin Marinas <catalin.marinas at arm.com>
> Cc: Will Deacon <Will.Deacon at arm.com>
> Cc: stable at vger.kernel.org # v4.10

Acked-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>

> ---
>  arch/arm64/mm/mmu.c | 34 ++++------------------------------
>  1 file changed, 4 insertions(+), 30 deletions(-)
>
> I'm aware that our hugtlbpage code has a similar issue. I'm looking into that
> now, and will address that with separate patches. It should be possible to use
> BBM there as it's a userspace mapping.
>

Are you looking into this issue as well? Or should I?


> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 17243e4..c391b1f 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -108,10 +108,8 @@ static bool pgattr_change_is_safe(u64 old, u64 new)
>  static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
>                                   unsigned long end, unsigned long pfn,
>                                   pgprot_t prot,
> -                                 phys_addr_t (*pgtable_alloc)(void),
> -                                 bool page_mappings_only)
> +                                 phys_addr_t (*pgtable_alloc)(void))
>  {
> -       pgprot_t __prot = prot;
>         pte_t *pte;
>
>         BUG_ON(pmd_sect(*pmd));
> @@ -129,18 +127,7 @@ static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
>         do {
>                 pte_t old_pte = *pte;
>
> -               /*
> -                * Set the contiguous bit for the subsequent group of PTEs if
> -                * its size and alignment are appropriate.
> -                */
> -               if (((addr | PFN_PHYS(pfn)) & ~CONT_PTE_MASK) == 0) {
> -                       if (end - addr >= CONT_PTE_SIZE && !page_mappings_only)
> -                               __prot = __pgprot(pgprot_val(prot) | PTE_CONT);
> -                       else
> -                               __prot = prot;
> -               }
> -
> -               set_pte(pte, pfn_pte(pfn, __prot));
> +               set_pte(pte, pfn_pte(pfn, prot));
>                 pfn++;
>
>                 /*
> @@ -159,7 +146,6 @@ static void alloc_init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
>                                   phys_addr_t (*pgtable_alloc)(void),
>                                   bool page_mappings_only)
>  {
> -       pgprot_t __prot = prot;
>         pmd_t *pmd;
>         unsigned long next;
>
> @@ -186,18 +172,7 @@ static void alloc_init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
>                 /* try section mapping first */
>                 if (((addr | next | phys) & ~SECTION_MASK) == 0 &&
>                       !page_mappings_only) {
> -                       /*
> -                        * Set the contiguous bit for the subsequent group of
> -                        * PMDs if its size and alignment are appropriate.
> -                        */
> -                       if (((addr | phys) & ~CONT_PMD_MASK) == 0) {
> -                               if (end - addr >= CONT_PMD_SIZE)
> -                                       __prot = __pgprot(pgprot_val(prot) |
> -                                                         PTE_CONT);
> -                               else
> -                                       __prot = prot;
> -                       }
> -                       pmd_set_huge(pmd, phys, __prot);
> +                       pmd_set_huge(pmd, phys, prot);
>
>                         /*
>                          * After the PMD entry has been populated once, we
> @@ -207,8 +182,7 @@ static void alloc_init_pmd(pud_t *pud, unsigned long addr, unsigned long end,
>                                                       pmd_val(*pmd)));
>                 } else {
>                         alloc_init_pte(pmd, addr, next, __phys_to_pfn(phys),
> -                                      prot, pgtable_alloc,
> -                                      page_mappings_only);
> +                                      prot, pgtable_alloc);
>
>                         BUG_ON(pmd_val(old_pmd) != 0 &&
>                                pmd_val(old_pmd) != pmd_val(*pmd));
> --
> 1.9.1
>