arm: Fix memory attribute inconsistencies when using fixmap

From: Jon Medhurst <[email protected]>

To cope with the variety in ARM architectures and configurations, the pagetable attributes for kernel memory are generated at runtime to match the system the kernel finds itself on. This calculated value is stored in pgprot_kernel.

However, when early fixmap support was added for ARM (commit a5f4c561b3b1) the attributes used for mappings were hard coded because pgprot_kernel is not set up early enough. Unfortunately, when fixmap is used after early boot this means the memory being mapped can have different attributes to existing mappings, potentially leading to unpredictable behaviour. A specific problem also exists due to the hard coded values not include the 'shareable' attribute which means on systems where this matters (e.g. those with multiple CPU clusters) the cache contents for a memory location can become inconsistent between CPUs.

To resolve these issues we change fixmap to use the same memory attributes (from pgprot_kernel) that the rest of the kernel uses. To enable this we need to refactor the initialisation code so build_mem_type_table() is called early enough. Note, that relies on early param parsing for memory type overrides passed via the kernel command line, so we need to make sure this call is still after parse_early_params().

Fixes a5f4c561b3b1 ("ARM: 8415/1: early fixmap support for earlycon")

Cc <[email protected]> # v4.3+

Signed-off-by Jon Medhurst <[email protected]>

[ardb: keep early_fixmap_init() before param parsing, for earlycon]

Signed-off-by Ard Biesheuvel <[email protected]>

---

arch/arm/mm/mmu.c +0 -1

 	if (pgprot_val(prot))
 		set_pte_at(NULL, vaddr, pte,
 			pfn_pte(phys >> PAGE_SHIFT, prot));

@@ -1616,7 +1621,6 @@ void __init paging_init(const struct machine_desc *mdesc)
 {
 	void *zero_page;
 
-	build_mem_type_table();
 	prepare_page_table();
 	map_lowmem();
 	memblock_set_current_limit(arm_lowmem_limit);

Quoted 32 lines · Ard Biesheuvel

From: Jon Medhurst <[email protected]>

To cope with the variety in ARM architectures and configurations, the pagetable attributes for kernel memory are generated at runtime to match the system the kernel finds itself on. This calculated value is stored in pgprot_kernel.

However, when early fixmap support was added for ARM (commit a5f4c561b3b1) the attributes used for mappings were hard coded because pgprot_kernel is not set up early enough. Unfortunately, when fixmap is used after early boot this means the memory being mapped can have different attributes to existing mappings, potentially leading to unpredictable behaviour. A specific problem also exists due to the hard coded values not include the 'shareable' attribute which means on systems where this matters (e.g. those with multiple CPU clusters) the cache contents for a memory location can become inconsistent between CPUs.

To resolve these issues we change fixmap to use the same memory attributes (from pgprot_kernel) that the rest of the kernel uses. To enable this we need to refactor the initialisation code so build_mem_type_table() is called early enough. Note, that relies on early param parsing for memory type overrides passed via the kernel command line, so we need to make sure this call is still after parse_early_params().

Fixes a5f4c561b3b1 ("ARM: 8415/1: early fixmap support for earlycon")

Cc <[email protected]> # v4.3+

Signed-off-by Jon Medhurst <[email protected]>

[ardb: keep early_fixmap_init() before param parsing, for earlycon]

Signed-off-by Ard Biesheuvel <[email protected]>

---

[...]

Quoted 8 lines

arch/arm/mm/mmu.c +0 -0

index 4e016d7f37b3..ec81a30479aa 100644
@@ -414,6 +414,11 @@ void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot)
                   FIXADDR_END);
      BUG_ON(idx >= __end_of_fixed_addresses);
 

arch/arm/mm/mmu.c void __set_fixmap(enum fixed_addresses idx, phys_addr_t phys, pgprot_t prot) · line 421

Quoted 5 lines

arch/arm/mm/mmu.c +5 -0

+     /* we only support device mappings until pgprot_kernel has been set */
+     if (WARN_ON(pgprot_val(prot) != pgprot_val(FIXMAP_PAGE_IO) &&
+                 pgprot_val(pgprot_kernel) == 0))
+             return;
+

Hmm, on return from this function, isn't the caller then going to try and access the memory at the fixmap address we didn't map, and so get a page fault? If so, would a BUG_ON here be better?

Quoted 12 lines

arch/arm/mm/mmu.c +0 -1

      if (pgprot_val(prot))
              set_pte_at(NULL, vaddr, pte,
                      pfn_pte(phys >> PAGE_SHIFT, prot));

@@ -1616,7 +1621,6 @@ void __init paging_init(const struct machine_desc *mdesc)
 {
      void *zero_page;
 
-     build_mem_type_table();
      prepare_page_table();
      map_lowmem();
      memblock_set_current_limit(arm_lowmem_limit);

arch/arm/mm/mmu.c void __init paging_init(const struct machine_desc *mdesc) · line 1648

Quoted 9 lines

arch/arm/mm/mmu.c +6 -0

@@ -1636,3 +1640,9 @@ void __init paging_init(const struct machine_desc *mdesc)
      empty_zero_page = virt_to_page(zero_page);
      __flush_dcache_page(NULL, empty_zero_page);
 }
+
+void __init early_mm_init(const struct machine_desc *mdesc)
+{
+     build_mem_type_table();
+     early_paging_init(mdesc);
+}

I tested this change with kprobes tests on TC2 (the setup that showed the original bug) and compile tested for a nommu device (I will boot test that in a bit as that's not quick to setup).

Also, with this patch we can now make early_paging_init() static now, and remove the extern...

arch/arm/kernel/setup.c +0 -1

index f5db5548ad42..8ad1e4414024 100644
@@ -80,7 +80,6 @@ __setup("fpe=", fpe_setup);
 
 extern void init_default_cache_policy(unsigned long);
 extern void paging_init(const struct machine_desc *desc);
-extern void early_paging_init(const struct machine_desc *);
 extern void adjust_lowmem_bounds(void);
 extern enum reboot_mode reboot_mode;
 extern void setup_dma_zone(const struct machine_desc *desc);

arch/arm/mm/mmu.c +1 -1

index ec81a30479aa..347cca965783 100644
@@ -1497,7 +1497,7 @@ pgtables_remap lpae_pgtables_remap_asm;
  * early_paging_init() recreates boot time page table setup, allowing machines
  * to switch over to a high (>4G) address space on LPAE systems
  */
-void __init early_paging_init(const struct machine_desc *mdesc)
+static void __init early_paging_init(const struct machine_desc *mdesc)
 {
        pgtables_remap *lpae_pgtables_remap;
        unsigned long pa_pgd;

On 4 April 2017 at 09:48, Jon Medhurst (Tixy) <[email protected]> wrote:

Quoted 3 lines

I tested this change with kprobes tests on TC2 (the setup that showed the original bug) and compile tested for a nommu device (I will boot test that in a bit as that's not quick to setup).

Quoted 3 lines

Also, with this patch we can now make early_paging_init() static now, and remove the extern...

Good point. If everything checks out, we should fold that in when submitting it to the patch tracker.

arch/arm/include/asm/mmu.h +4 -0

index a5b47421059d..80dfeb8ce8f2 100644
@@ -1,6 +1,8 @@
 #ifndef __ARM_MMU_H
 #define __ARM_MMU_H
 
+struct machine_desc;
+
 #ifdef CONFIG_MMU
 
 typedef struct {

@@ -24,6 +26,8 @@ typedef struct {
 #define ASID(mm)	(0)
 #endif
 
+void early_mm_init(const struct machine_desc *mdesc);
+
 #else
 
 /*

Quoted 23 lines · Ard Biesheuvel

arch/arm/include/asm/mmu.h +4 -0

index a5b47421059d..80dfeb8ce8f2 100644
@@ -1,6 +1,8 @@
 #ifndef __ARM_MMU_H
 #define __ARM_MMU_H
 
+struct machine_desc;
+
 #ifdef CONFIG_MMU
 
 typedef struct {

@@ -24,6 +26,8 @@ typedef struct {
 #define ASID(mm)	(0)
 #endif
 
+void early_mm_init(const struct machine_desc *mdesc);
+
 #else
 
 /*

arch/arm/include/asm/mmu.h typedef struct { · line 43

Quoted 5 lines

arch/arm/include/asm/mmu.h +2 -0

@@ -35,6 +39,8 @@ typedef struct {
 	unsigned long	end_brk;
 } mm_context_t;
 
+static inline void early_mm_init(const struct machine_desc *mdesc) { }
+

Why are we stuffing this into mmu.h? mmu.h is not the header file for arch/arm/mm/mmu.c - although they have the same base filename, they are not really related.

asm/mmu.h is supposed to be a kernel-wide include file for the mm_context_t thing, and should not be filled with arch-private stuff.

Please find a better location for this.

Quoted 37 lines · Russell King - ARM Linux

Quoted 23 lines · Ard Biesheuvel

arch/arm/include/asm/mmu.h +4 -0

index a5b47421059d..80dfeb8ce8f2 100644
@@ -1,6 +1,8 @@
 #ifndef __ARM_MMU_H
 #define __ARM_MMU_H
 
+struct machine_desc;
+
 #ifdef CONFIG_MMU
 
 typedef struct {

@@ -24,6 +26,8 @@ typedef struct {
 #define ASID(mm)   (0)
 #endif
 
+void early_mm_init(const struct machine_desc *mdesc);
+
 #else
 
 /*

arch/arm/include/asm/mmu.h typedef struct { · line 43

Quoted 5 lines

arch/arm/include/asm/mmu.h +2 -0

@@ -35,6 +39,8 @@ typedef struct {
    unsigned long   end_brk;
 } mm_context_t;
 
+static inline void early_mm_init(const struct machine_desc *mdesc) { }
+

Why are we stuffing this into mmu.h? mmu.h is not the header file for arch/arm/mm/mmu.c - although they have the same base filename, they are not really related.

asm/mmu.h is supposed to be a kernel-wide include file for the mm_context_t thing, and should not be filled with arch-private stuff.

Quoted 1 line

Please find a better location for this.

In arch/arm/mm/mm.h ?

Or as early_mm_init() is replacing early_paging_init() in setup.c, and that is currently handled by a local extern statement, keeping doing the same? That keeps the diff neater which is a plus for a patch that should be backported to stable kernels.

To cope with the variety in ARM architectures and configurations, the pagetable attributes for kernel memory are generated at runtime to match the system the kernel finds itself on. This calculated value is stored in pgprot_kernel.

However, when early fixmap support was added for ARM (commit a5f4c561b3b1) the attributes used for mappings were hard coded because pgprot_kernel is not set up early enough. Unfortunately, when fixmap is used after early boot this means the memory being mapped can have different attributes to existing mappings, potentially leading to unpredictable behaviour. A specific problem also exists due to the hard coded values not include the 'shareable' attribute which means on systems where this matters (e.g. those with multiple CPU clusters) the cache contents for a memory location can become inconsistent between CPUs.

To resolve these issues we change fixmap to use the same memory attributes (from pgprot_kernel) that the rest of the kernel uses. To enable this we need to refactor the initialisation code so build_mem_type_table() is called early enough. Note, that relies on early param parsing for memory type overrides passed via the kernel command line, so we need to make sure this call is still after parse_early_params().

Hi,

Quoted 16 lines · Jon Medhurst (Tixy)

To cope with the variety in ARM architectures and configurations, the pagetable attributes for kernel memory are generated at runtime to match the system the kernel finds itself on. This calculated value is stored in pgprot_kernel.

However, when early fixmap support was added for ARM (commit a5f4c561b3b1) the attributes used for mappings were hard coded because pgprot_kernel is not set up early enough. Unfortunately, when fixmap is used after early boot this means the memory being mapped can have different attributes to existing mappings, potentially leading to unpredictable behaviour. A specific problem also exists due to the hard coded values not include the 'shareable' attribute which means on systems where this matters (e.g. those with multiple CPU clusters) the cache contents for a memory location can become inconsistent between CPUs.

Quoted 7 lines

To resolve these issues we change fixmap to use the same memory attributes (from pgprot_kernel) that the rest of the kernel uses. To enable this we need to refactor the initialisation code so build_mem_type_table() is called early enough. Note, that relies on early param parsing for memory type overrides passed via the kernel command line, so we need to make sure this call is still after parse_early_params().

Tested-by afzal mohammed <[email protected]>

with an emphasis on no-MMU's