arm64: mm: rework page table creation

Currently __set_fixmap_offset is a macro function which has a local variable called 'addr'. If a caller passes a 'phys' parameter which is derived from a variable also called 'addr', the local variable will shadow this, and the compiler will complain about the use of an uninitialized variable.

It is likely that fixmap users may use the name 'addr' for variables that may be directly passed to __set_fixmap_offset, or that may be indirectly generated via other macros. Rather than placing the burden on callers to avoid the name 'addr', this patch changes __set_fixmap_offset into a static inline function, avoiding namespace collisions.

Signed-off-by Mark Rutland <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Jeremy Linton <[email protected]>

Cc Laura Abbott <[email protected]>

Cc Will Deacon <[email protected]>

---
 include/asm-generic/fixmap.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

include/asm-generic/fixmap.h +7 -7

index 1cbb833..f9c27b6 100644
@@ -70,13 +70,13 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr)
 #endif
 
 /* Return a pointer with offset calculated */
-#define __set_fixmap_offset(idx, phys, flags)		      \
-({							      \
-	unsigned long addr;				      \
-	__set_fixmap(idx, phys, flags);			      \
-	addr = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \
-	addr;						      \
-})
+static inline unsigned long __set_fixmap_offset(enum fixed_addresses idx,
+						phys_addr_t phys,
+						pgprot_t flags)
+{
+	__set_fixmap(idx, phys, flags);
+	return fix_to_virt(idx) + (phys & (PAGE_SIZE - 1));
+}
 
 #define set_fixmap_offset(idx, phys) \
 	__set_fixmap_offset(idx, phys, FIXMAP_PAGE_NORMAL)

Signature · 1 line

1.9.1

Hi Arnd,

Sorry to poke during the merge window.

Are you happy with the below patch, and if so, would you be happy for this to go via the arm64 tree?

If possible, I'd like to be able to place the series on a stable branch come -rc1, so that can be used as the base for other work (e.g. [1]), and so that we can get it into -next.

Everything else has the appropriate acks collected.

Quoted 11 lines · On Tuesday 19 January 2016 11:55:40 Mark Rutland wrote:

Hi Arnd,

Sorry to poke during the merge window.

Are you happy with the below patch, and if so, would you be happy for this to go via the arm64 tree?

If possible, I'd like to be able to place the series on a stable branch come -rc1, so that can be used as the base for other work (e.g. [1]), and so that we can get it into -next.

Quoted 3 lines

Everything else has the appropriate acks collected.

Yes, please merge it through arm64

Acked-by Arnd Bergmann <[email protected]>

Currently __set_fixmap_offset is a macro function which has a local variable called 'addr'. If a caller passes a 'phys' parameter which is derived from a variable also called 'addr', the local variable will shadow this, and the compiler will complain about the use of an uninitialized variable.

It is likely that fixmap users may use the name 'addr' for variables that may be directly passed to __set_fixmap_offset, or that may be indirectly generated via other macros. Rather than placing the burden on callers to avoid the name 'addr', this patch changes __set_fixmap_offset into a static inline function, avoiding namespace collisions.

Signed-off-by Mark Rutland <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Jeremy Linton <[email protected]>

Cc Laura Abbott <[email protected]>

Cc Will Deacon <[email protected]>

---
 include/asm-generic/fixmap.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

arch/arm64/mm/proc.S ENTRY(cpu_do_switch_mm) · line 153

Quoted 12 lines · Mark Rutland

arch/arm64/mm/proc.S +12 -0

+	.pushsection ".idmap.text", "ax"
+/*
+ * void idmap_cpu_replace_ttbr1(phys_addr_t new_pgd, phys_addr_t reserved_pgd)
+ *
+ * This is the low-level counterpart to cpu_replace_ttbr1, and should not be
+ * called by anything else. It can only be executed from a TTBR0 mapping.
+ */
+ENTRY(idmap_cpu_replace_ttbr1)
+	mrs	x2, daif
+	msr	daifset, #0xf
+
+	msr	ttbr1_el1, x1

Would it work to avoid the second argument and only use adrp, now that empty_zero_page is at a fixed offset relative to this function?

As a prepratory step to allow us to allocate early page tables from unmapped memory using memblock_alloc, add new p??_fixmap* functions that can be used to walk page tables outside of the linear mapping by using fixmap slots.

Signed-off-by Mark Rutland <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Jeremy Linton <[email protected]>

Cc Laura Abbott <[email protected]>

Cc Will Deacon <[email protected]>

---
  arch/arm64/include/asm/fixmap.h  | 10 ++++++++++
  arch/arm64/include/asm/pgtable.h | 26 ++++++++++++++++++++++++++
  2 files changed, 36 insertions(+)

arch/arm64/include/asm/fixmap.h +10 -0

index 3097045..1a617d4 100644
@@ -62,6 +62,16 @@ enum fixed_addresses {
 
  	FIX_BTMAP_END = __end_of_permanent_fixed_addresses,
  	FIX_BTMAP_BEGIN = FIX_BTMAP_END + TOTAL_FIX_BTMAPS - 1,
+
+	/*
+	 * Used for kernel page table creation, so unmapped memory may be used
+	 * for tables.
+	 */
+	FIX_PTE,
+	FIX_PMD,
+	FIX_PUD,
+	FIX_PGD,
+
  	__end_of_fixed_addresses
  };
 

arch/arm64/include/asm/pgtable.h +23 -0

index f5742db..824e7f0 100644
@@ -57,6 +57,7 @@
 
  #ifndef __ASSEMBLY__
 
+#include <asm/fixmap.h>
  #include <linux/mmdebug.h>
 
  extern void __pte_error(const char *file, int line, unsigned long val);

@@ -442,6 +443,10 @@ static inline phys_addr_t pmd_page_paddr(pmd_t pmd)
  #define pte_unmap(pte)			do { } while (0)
  #define pte_unmap_nested(pte)		do { } while (0)
 
+#define pte_fixmap(addr)		((pte_t *)set_fixmap_offset(FIX_PTE, addr))
+#define pte_fixmap_offset(pmd, addr)	pte_fixmap(pte_offset_phys(pmd, addr))
+#define pte_fixmap_unmap()		clear_fixmap(FIX_PTE)
+
  #define pmd_page(pmd)		pfn_to_page(__phys_to_pfn(pmd_val(pmd) & PHYS_MASK))
 
  /*

@@ -481,12 +486,21 @@ static inline phys_addr_t pud_page_paddr(pud_t pud)
  #define pmd_offset_phys(dir, addr)	(pud_page_paddr(*(dir)) + pmd_index(addr) * sizeof(pmd_t))
  #define pmd_offset(dir, addr)		((pmd_t *)__va(pmd_offset_phys((dir), (addr))))
 
+#define pmd_fixmap(addr)		((pmd_t *)set_fixmap_offset(FIX_PMD, addr))
+#define pmd_fixmap_offset(pud, addr)	pmd_fixmap(pmd_offset_phys(pud, addr))
+#define pmd_fixmap_unmap()		clear_fixmap(FIX_PMD)
+
  #define pud_page(pud)		pfn_to_page(__phys_to_pfn(pud_val(pud) & PHYS_MASK))
 
  #else
 
  #define pud_page_paddr(pud)	({ BUILD_BUG(); 0; })
 
+/* Match pmd_offset folding in <asm/generic/pgtable-nopmd.h> */
+#define pmd_fixmap(addr)		NULL
+#define pmd_fixmap_offset(pudp, addr)	((pmd_t *)pudp)
+#define pmd_fixmap_unmap()
+
  #endif	/* CONFIG_PGTABLE_LEVELS > 2 */
 
  #if CONFIG_PGTABLE_LEVELS > 3

@@ -519,12 +533,21 @@ static inline phys_addr_t pgd_page_paddr(pgd_t pgd)
  #define pud_offset_phys(dir, addr)	(pgd_page_paddr(*(dir)) + pud_index(addr) * sizeof(pud_t))
  #define pud_offset(dir, addr)		((pud_t *)__va(pud_offset_phys((dir), (addr))))
 
+#define pud_fixmap(addr)		((pud_t *)set_fixmap_offset(FIX_PUD, addr))
+#define pud_fixmap_offset(pgd, addr)	pud_fixmap(pmd_offset_phys(pgd, addr))
+#define pud_fixmap_unmap()		clear_fixmap(FIX_PUD)
+
  #define pgd_page(pgd)		pfn_to_page(__phys_to_pfn(pgd_val(pgd) & PHYS_MASK))
 
  #else
 
  #define pgd_page_paddr(pgd)	({ BUILD_BUG(); 0;})
 
+/* Match pud_offset folding in <asm/generic/pgtable-nopud.h> */
+#define pud_fixmap(addr)		NULL
+#define pud_fixmap_offset(pgdp, addr)	((pud_t *)pgdp)
+#define pud_fixmap_unmap()
+
  #endif  /* CONFIG_PGTABLE_LEVELS > 3 */
 
  #define pgd_ERROR(pgd)		__pgd_error(__FILE__, __LINE__, pgd_val(pgd))

Quoted 15 lines · Mark Rutland

As a prepratory step to allow us to allocate early page tables from unmapped memory using memblock_alloc, add new p??_fixmap* functions that can be used to walk page tables outside of the linear mapping by using fixmap slots.

Signed-off-by Mark Rutland <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Jeremy Linton <[email protected]>

Cc Laura Abbott <[email protected]>

Cc Will Deacon <[email protected]>

---
 arch/arm64/include/asm/fixmap.h  | 10 ++++++++++
 arch/arm64/include/asm/pgtable.h | 26 ++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

arch/arm64/include/asm/pgtable.h static inline phys_addr_t pgd_page_paddr(pgd_t pgd) · line 566

Quoted 2 lines

arch/arm64/include/asm/pgtable.h +2 -0

+#define pgd_fixmap(addr)		((pgd_t *)set_fixmap_offset(FIX_PGD, addr))
+#define pgd_fixmap_unmap()		clear_fixmap(FIX_PGD)

Bikeshed: p??_fixmap_offset doesn't make it obvious that this is an operation with a side effect. It seems more similar to p??_offset_kernel which is read only. Perhaps it's the lack of set/map in the name.

As a prepratory step to allow us to allocate early page tables form

unmapped memory using memblock_alloc, modify the __create_mapping callees to map and unmap the tables they modify using fixmap entries.

All but the top-level pgd initialisation is performed via the fixmap. Subsequent patches will inject the pgd physical address, and migrate to using the FIX_PGD slot.

Signed-off-by Mark Rutland <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Catalin Marinas <[email protected]>

Cc Jeremy Linton <[email protected]>

Cc Laura Abbott <[email protected]>

Cc Will Deacon <[email protected]>

---
  arch/arm64/include/asm/pgtable.h |  2 +-
  arch/arm64/mm/mmu.c              | 63 ++++++++++++++++++++++++++--------------
  2 files changed, 42 insertions(+), 23 deletions(-)

arch/arm64/include/asm/pgtable.h +0 -0

index 824e7f0..6fbf9fa 100644

arch/arm64/include/asm/pgtable.h +0 -0

  #define pud_fixmap_unmap()		clear_fixmap(FIX_PUD)
 
  #define pgd_page(pgd)		pfn_to_page(__phys_to_pfn(pgd_val(pgd) & PHYS_MASK))

arch/arm64/mm/mmu.c +39 -18

index 8879aed..bf09d44 100644
@@ -63,19 +63,30 @@ pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
  }
  EXPORT_SYMBOL(phys_mem_access_prot);
 
-static void __init *early_pgtable_alloc(void)
+static phys_addr_t __init early_pgtable_alloc(void)
  {
  	phys_addr_t phys;
  	void *ptr;
 
  	phys = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
  	BUG_ON(!phys);
-	ptr = __va(phys);
+
+	/*
+	 * The FIX_{PGD,PUD,PMD} slots may be in active use, but the FIX_PTE
+	 * slot will be free, so we can (ab)use the FIX_PTE slot to initialise
+	 * any level of table.
+	 */
+	ptr = pte_fixmap(phys);
+
  	memset(ptr, 0, PAGE_SIZE);
 
-	/* Ensure the zeroed page is visible to the page table walker */
-	dsb(ishst);
-	return ptr;
+	/*
+	 * Implicit barriers also ensure the zeroed page is visible to the page
+	 * table walker
+	 */
+	pte_fixmap_unmap();
+
+	return phys;
  }
 
  /*

@@ -99,24 +110,28 @@ static void split_pmd(pmd_t *pmd, pte_t *pte)
  static void alloc_init_pte(pmd_t *pmd, unsigned long addr,
  				  unsigned long end, unsigned long pfn,
  				  pgprot_t prot,
-				  void *(*pgtable_alloc)(void))
+				  phys_addr_t (*pgtable_alloc)(void))
  {
  	pte_t *pte;
 
  	if (pmd_none(*pmd) || pmd_sect(*pmd)) {
-		pte = pgtable_alloc();
+		phys_addr_t pte_phys = pgtable_alloc();
+		pte = pte_fixmap(pte_phys);
  		if (pmd_sect(*pmd))
  			split_pmd(pmd, pte);
-		__pmd_populate(pmd, __pa(pte), PMD_TYPE_TABLE);
+		__pmd_populate(pmd, pte_phys, PMD_TYPE_TABLE);
  		flush_tlb_all();
+		pte_fixmap_unmap();
  	}
  	BUG_ON(pmd_bad(*pmd));
 
-	pte = pte_offset_kernel(pmd, addr);
+	pte = pte_fixmap_offset(pmd, addr);
  	do {
  		set_pte(pte, pfn_pte(pfn, prot));
  		pfn++;
  	} while (pte++, addr += PAGE_SIZE, addr != end);
+
+	pte_fixmap_unmap();
  }
 
  static void split_pud(pud_t *old_pud, pmd_t *pmd)

@@ -134,7 +149,7 @@ static void split_pud(pud_t *old_pud, pmd_t *pmd)
  static void alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
  				  unsigned long addr, unsigned long end,
  				  phys_addr_t phys, pgprot_t prot,
-				  void *(*pgtable_alloc)(void))
+				  phys_addr_t (*pgtable_alloc)(void))
  {
  	pmd_t *pmd;
  	unsigned long next;

@@ -143,7 +158,8 @@ static void alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
  	 * Check for initial section mappings in the pgd/pud and remove them.
  	 */
  	if (pud_none(*pud) || pud_sect(*pud)) {
-		pmd = pgtable_alloc();
+		phys_addr_t pmd_phys = pgtable_alloc();
+		pmd = pmd_fixmap(pmd_phys);
  		if (pud_sect(*pud)) {
  			/*
  			 * need to have the 1G of mappings continue to be

@@ -151,12 +167,13 @@ static void alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
  			 */
  			split_pud(pud, pmd);
  		}
-		pud_populate(mm, pud, pmd);
+		__pud_populate(pud, pmd_phys, PUD_TYPE_TABLE);
  		flush_tlb_all();
+		pmd_fixmap_unmap();
  	}
  	BUG_ON(pud_bad(*pud));
 
-	pmd = pmd_offset(pud, addr);
+	pmd = pmd_fixmap_offset(pud, addr);
  	do {
  		next = pmd_addr_end(addr, end);
  		/* try section mapping first */

@@ -182,6 +199,8 @@ static void alloc_init_pmd(struct mm_struct *mm, pud_t *pud,
  		}
  		phys += next - addr;
  	} while (pmd++, addr = next, addr != end);
+
+	pmd_fixmap_unmap();
  }
 
  static inline bool use_1G_block(unsigned long addr, unsigned long next,

@@ -199,18 +218,18 @@ static inline bool use_1G_block(unsigned long addr, unsigned long next,
  static void alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
  				  unsigned long addr, unsigned long end,
  				  phys_addr_t phys, pgprot_t prot,
-				  void *(*pgtable_alloc)(void))
+				  phys_addr_t (*pgtable_alloc)(void))
  {
  	pud_t *pud;
  	unsigned long next;
 
  	if (pgd_none(*pgd)) {
-		pud = pgtable_alloc();
-		pgd_populate(mm, pgd, pud);
+		phys_addr_t pud_phys = pgtable_alloc();
+		__pgd_populate(pgd, pud_phys, PUD_TYPE_TABLE);
  	}
  	BUG_ON(pgd_bad(*pgd));
 
-	pud = pud_offset(pgd, addr);
+	pud = pud_fixmap_offset(pgd, addr);
  	do {
  		next = pud_addr_end(addr, end);
 

@@ -243,6 +262,8 @@ static void alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
  		}
  		phys += next - addr;
  	} while (pud++, addr = next, addr != end);
+
+	pud_fixmap_unmap();
  }
 
  /*

@@ -252,7 +273,7 @@ static void alloc_init_pud(struct mm_struct *mm, pgd_t *pgd,
  static void  __create_mapping(struct mm_struct *mm, pgd_t *pgd,
  				    phys_addr_t phys, unsigned long virt,
  				    phys_addr_t size, pgprot_t prot,
-				    void *(*pgtable_alloc)(void))
+				    phys_addr_t (*pgtable_alloc)(void))
  {
  	unsigned long addr, length, end, next;
 

@@ -275,14 +296,12 @@ static void  __create_mapping(struct mm_struct *mm, pgd_t *pgd,
  	} while (pgd++, addr = next, addr != end);

Quoted 1 line · Mark Rutland

As a prepratory step to allow us to allocate early page tables form

"page tables from"

arch/arm64/include/asm/pgtable.h static inline phys_addr_t pgd_page_paddr(pgd_t pgd) · line 537

Quoted 2 lines

arch/arm64/include/asm/pgtable.h +1 -1

-#define pud_fixmap_offset(pgd, addr)	pud_fixmap(pmd_offset_phys(pgd, addr))
+#define pud_fixmap_offset(pgd, addr)	pud_fixmap(pud_offset_phys(pgd, addr))

Was this supposed to be folded into a previous patch?

arch/arm64/mm/mmu.c static void __create_mapping(struct mm_struct *mm, pgd_t *pgd, · line 303

Quoted 8 lines

arch/arm64/mm/mmu.c +1 -3

-static void *late_pgtable_alloc(void)
+static phys_addr_t late_pgtable_alloc(void)
 {
 	void *ptr = (void *)__get_free_page(PGALLOC_GFP);
 	BUG_ON(!ptr);
-
-	/* Ensure the zeroed page is visible to the page table walker */
 	dsb(ishst);

This dropped the comment but not the actual barrier, was that intentional?

Hi all,

This series reworks the arm64 early page table code, in order to:

(a) Avoid issues with potentially-conflicting TTBR1 TLB entries (as raised in Jeremy's thread [1]). This can happen when splitting/merging sections or contiguous ranges, and per a pessimistic reading of the ARM ARM may happen for changes to other fields in translation table entries.

(b) Allow for more complex page table creation early on, with tables created with fine-grained permissions as early as possible. In the cases where we currently use fine-grained permissions (e.g. DEBUG_RODATA and marking .init as non-executable), this is required for the same reasons as (a), as we must ensure that changes to page tables do not split/merge sections or contiguous regions for memory in active use.

(c) Avoid edge cases where we need to allocate memory before a sufficient proportion of the early linear map is in place to accommodate allocations.

This series:

* Introduces the necessary infrastructure to safely swap TTBR1_EL1 (i.e. without risking conflicting TLB entries being allocated). The arm64 KASAN code is migrated to this.

* Adds helpers to walk page tables by physical address, independent of the linear mapping, and modifies __create_mapping and friends to relying on a new set of FIX_{PGD,PUD,PMD,PTE} to map tables as required for modification.

* Removes the early memblock limit, now that create_mapping does not rely on the early linear map. This solves (c), and allows for (b).

* Generates an entirely new set of kernel page tables with fine-grained (i.e. page-level) permission boundaries, which can then be safely installed. These are created with sufficient granularity such that later changes (currently only fixup_init) will not split/merge sections or contiguous regions, and can follow a break-before-make approach without affecting the rest of the page tables.

There are still opportunities for improvement:

* BUG() when splitting sections or creating overlapping entries in create_mapping, as these both indicate serious bugs in kernel page table creation.

This will require rework to the EFI runtime services pagetable creation, as for >4K page kernels EFI memory descriptors may share pages (and currently such overlap is assumed to be benign).

* Use ROX mappings for the kernel text and rodata when creating the new tables. This avoiding potential conflicts from changes to translation tables, and giving us better protections earlier.

Currently the alternatives patching code relies on being able to use the kernel mapping to update the text. We cannot rely on any text which itself may be patched, and updates may straddle page boundaries, so this is non-trivial.

* Clean up usage of swapper_pg_dir so we can switch to the new tables without having to reuse the existing pgd. This will allow us to free the original pgd (i.e. we can free all the initial tables in one go).

Any and all feedback is welcome.

This series is based on today's arm64 [2] for-next/core branch (commit c9cd0ed925c0b927), and this version is tagged as arm64-pagetable-rework-20160104 while the latest version should be in the unstable branch arm64/pagetable-rework in my git repo [3].

Since v1 [4] (tagged arm64-pagetable-rework-20151209):
* Drop patches taken into the arm64 tree.
* Rebase to arm64 for-next/core.
* Copy early KASAN tables.
* Fix KASAN pgd manipulation.
* Specialise allocators for page tables, in function and naming.
* Update comments.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386178.html
[2] git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git
[3] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git
[4] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/392292.html

Mark Rutland (18):
asm-generic: make __set_fixmap_offset a static inline
arm64: mm: specialise pagetable allocators
arm64: mm: place empty_zero_page in bss
arm64: unify idmap removal
arm64: unmap idmap earlier
arm64: add function to install the idmap
arm64: mm: add code to safely replace TTBR1_EL1
arm64: kasan: avoid TLB conflicts
arm64: mm: move pte_* macros
arm64: mm: add functions to walk page tables by PA
arm64: mm: avoid redundant __pa(__va(x))
arm64: mm: add __{pud,pgd}_populate
arm64: mm: add functions to walk tables in fixmap
arm64: mm: use fixmap when creating page tables
arm64: mm: allocate pagetables anywhere
arm64: mm: allow passing a pgdir to alloc_init_*
arm64: ensure _stext and _etext are page-aligned
arm64: mm: create new fine-grained mappings at boot

  arch/arm64/include/asm/fixmap.h      |  10 ++
  arch/arm64/include/asm/kasan.h       |   3 +
  arch/arm64/include/asm/mmu_context.h |  63 ++++++-
  arch/arm64/include/asm/pgalloc.h     |  26 ++-
  arch/arm64/include/asm/pgtable.h     |  87 +++++++---
  arch/arm64/kernel/head.S             |   1 +
  arch/arm64/kernel/setup.c            |   7 +
  arch/arm64/kernel/smp.c              |   4 +-
  arch/arm64/kernel/suspend.c          |  20 +--
  arch/arm64/kernel/vmlinux.lds.S      |   5 +-
  arch/arm64/mm/kasan_init.c           |  32 ++--
  arch/arm64/mm/mmu.c                  | 311 ++++++++++++++++++-----------------
  arch/arm64/mm/proc.S                 |  27 +++
  include/asm-generic/fixmap.h         |  14 +-
  14 files changed, 381 insertions(+), 229 deletions(-)

Quoted 15 lines · Mark Rutland

Hi all,

This series reworks the arm64 early page table code, in order to:

(a) Avoid issues with potentially-conflicting TTBR1 TLB entries (as raised in Jeremy's thread [1]). This can happen when splitting/merging sections or contiguous ranges, and per a pessimistic reading of the ARM ARM may happen for changes to other fields in translation table entries.

(b) Allow for more complex page table creation early on, with tables created with fine-grained permissions as early as possible. In the cases where we currently use fine-grained permissions (e.g. DEBUG_RODATA and marking .init as non-executable), this is required for the same reasons as (a), as we must ensure that changes to page tables do not split/merge sections or contiguous regions for memory in active use.

Quoted 6 lines

There are still opportunities for improvement:

* BUG() when splitting sections or creating overlapping entries in create_mapping, as these both indicate serious bugs in kernel page table creation.

Quoted 3 lines

This will require rework to the EFI runtime services pagetable creation, as for >4K page kernels EFI memory descriptors may share pages (and currently such overlap is assumed to be benign).

Given the split_{pmd,pud} were added for DEBUG_RODATA, is there any reason those can't be dropped now since it sounds like the EFI problem is for overlapping entries and not splitting?

This series points out that my attempt to allow set_memory_* to work on regular kernel memory[1] is broken right now because it breaks down the larger block sizes.

Do you have any suggestions for a cleaner approach
short of requiring all memory mapped with 4K pages? The only solution I see right now is having a separate copy of page tables to switch to. Any idea other idea I come up with would have problems if we tried to invalidate an entry before breaking it down.

Thanks,
Laura

[1]https://lkml.kernel.org/g/<[email protected]>

Quoted 16 lines · Laura Abbott

Quoted 15 lines · Mark Rutland

Hi all,

This series reworks the arm64 early page table code, in order to:

(a) Avoid issues with potentially-conflicting TTBR1 TLB entries (as raised in Jeremy's thread [1]). This can happen when splitting/merging sections or contiguous ranges, and per a pessimistic reading of the ARM ARM may happen for changes to other fields in translation table entries.

(b) Allow for more complex page table creation early on, with tables created with fine-grained permissions as early as possible. In the cases where we currently use fine-grained permissions (e.g. DEBUG_RODATA and marking .init as non-executable), this is required for the same reasons as (a), as we must ensure that changes to page tables do not split/merge sections or contiguous regions for memory in active use.

[...]

Quoted 6 lines

Quoted 6 lines

There are still opportunities for improvement:

* BUG() when splitting sections or creating overlapping entries in create_mapping, as these both indicate serious bugs in kernel page table creation.

Quoted 7 lines

Quoted 3 lines

This will require rework to the EFI runtime services pagetable creation, as for >4K page kernels EFI memory descriptors may share pages (and currently such overlap is assumed to be benign).

Given the split_{pmd,pud} were added for DEBUG_RODATA, is there any reason those can't be dropped now since it sounds like the EFI problem is for overlapping entries and not splitting?

Good point. I think they can be removed.

I'll take a look into that.

Quoted 3 lines

This series points out that my attempt to allow set_memory_* to work on regular kernel memory[1] is broken right now because it breaks down the larger block sizes.

What's the rationale for set_memory_* on kernel mappings? I see "security", but I couldn't figure out a concrete use-case. Is there any example of a subsystem that wants to use this?

For statically-allocated data, an alternative approach would be for such memory to be mapped with minimal permissions from the outset (e.g. being placed in .rodata), and when elevated permissions are required a (temporary) memremap'd alias could be used, like what patch_map does to modify ROX kernel/module text.

For dynamically-allocated data, we could create (minimal permission) mappings in the vmalloc region and pass those around. The linear map alias would still be writeable, but as the offset between the two isn't linear (and the owner of that allocation doesn't have to know/care about the linear map address), it would be much harder to find the linear map address to attack. An alias with elevated permissions could be used as required, or if it's a one-time RW->RO switch, the mapping could me modified in-place as the granularity wouldn't change.

Quoted 6 lines

Do you have any suggestions for a cleaner approach
short of requiring all memory mapped with 4K pages? The only solution I see right now is having a separate copy of page tables to switch to. Any idea other idea I come up with would have problems if we tried to invalidate an entry before breaking it down.

The other option I looked into was to have a completely independent TTBR0 mapping (like the idmap or efi runtime tables), and have that map code for modifying page tables. That way you could modify the tables in-place (with TTBR1 disabled for the duration of the modification).

That ended up having its own set of problems, as you could only rely on self-contained position independent code, which ruled out most kernel APIs (including locking/atomic primitives due to debug paths). That gets worse when secondaries are online and you have to synchronise those disabling/invalidating/enabling the TTBR1 mapping.

Other than that I haven't managed to come up with other functional ideas. The RCU-like approach is the cleanest I've found so far.

Quoted 7 lines

Quoted 3 lines

This series points out that my attempt to allow set_memory_* to work on regular kernel memory[1] is broken right now because it breaks down the larger block sizes.

What's the rationale for set_memory_* on kernel mappings? I see "security", but I couldn't figure out a concrete use-case. Is there any example of a subsystem that wants to use this?

From the description, it sounded like this was possibly new work but the eBPF interpreter currently supports setting a page read only via set_memory_ro (see 60a3b2253c413cf601783b070507d7dd6620c954 "net: bpf: make eBPF interpreter images read-only") so it's not unheard of.

Quoted 6 lines

For statically-allocated data, an alternative approach would be for such memory to be mapped with minimal permissions from the outset (e.g. being placed in .rodata), and when elevated permissions are required a (temporary) memremap'd alias could be used, like what patch_map does to modify ROX kernel/module text.

Quoted 8 lines

For dynamically-allocated data, we could create (minimal permission) mappings in the vmalloc region and pass those around. The linear map alias would still be writeable, but as the offset between the two isn't linear (and the owner of that allocation doesn't have to know/care about the linear map address), it would be much harder to find the linear map address to attack. An alias with elevated permissions could be used as required, or if it's a one-time RW->RO switch, the mapping could me modified in-place as the granularity wouldn't change.

This would work for new features but probably not for existing features such as the eBPF interpreter.

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

This series points out that my attempt to allow set_memory_* to work on regular kernel memory[1] is broken right now because it breaks down the larger block sizes.

What's the rationale for set_memory_* on kernel mappings? I see "security", but I couldn't figure out a concrete use-case. Is there any example of a subsystem that wants to use this?

Quoted 5 lines

From the description, it sounded like this was possibly new work but
the eBPF interpreter currently supports setting a page read only via set_memory_ro (see 60a3b2253c413cf601783b070507d7dd6620c954 "net: bpf: make eBPF interpreter images read-only") so it's not unheard of.

Oh. For some reason I thought that used the vmalloc area, but evidently I was mistaken.

That is unfortunate, it would be good to protect the JITed code.

Quoted 15 lines

Quoted 14 lines

For statically-allocated data, an alternative approach would be for such memory to be mapped with minimal permissions from the outset (e.g. being placed in .rodata), and when elevated permissions are required a (temporary) memremap'd alias could be used, like what patch_map does to modify ROX kernel/module text.

For dynamically-allocated data, we could create (minimal permission) mappings in the vmalloc region and pass those around. The linear map alias would still be writeable, but as the offset between the two isn't linear (and the owner of that allocation doesn't have to know/care about the linear map address), it would be much harder to find the linear map address to attack. An alias with elevated permissions could be used as required, or if it's a one-time RW->RO switch, the mapping could me modified in-place as the granularity wouldn't change.

Quoted 2 lines

This would work for new features but probably not for existing features such as the eBPF interpreter.

Sure.

For eBPF it might be possible to rework the code to support using separate aliases, but that's probably not going to be easy and that probably works against some performance requirement. :/

Quoted 37 lines · Mark Rutland

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

This series points out that my attempt to allow set_memory_* to work on regular kernel memory[1] is broken right now because it breaks down the larger block sizes.

What's the rationale for set_memory_* on kernel mappings? I see "security", but I couldn't figure out a concrete use-case. Is there any example of a subsystem that wants to use this?

Quoted 5 lines

From the description, it sounded like this was possibly new work but the eBPF interpreter currently supports setting a page read only via set_memory_ro (see 60a3b2253c413cf601783b070507d7dd6620c954 "net: bpf: make eBPF interpreter images read-only") so it's not unheard of.

Oh. For some reason I thought that used the vmalloc area, but evidently I was mistaken.

That is unfortunate, it would be good to protect the JITed code.

Quoted 17 lines

Quoted 6 lines

For statically-allocated data, an alternative approach would be for such memory to be mapped with minimal permissions from the outset (e.g. being placed in .rodata), and when elevated permissions are required a (temporary) memremap'd alias could be used, like what patch_map does to modify ROX kernel/module text.

Quoted 8 lines

For dynamically-allocated data, we could create (minimal permission) mappings in the vmalloc region and pass those around. The linear map alias would still be writeable, but as the offset between the two isn't linear (and the owner of that allocation doesn't have to know/care about the linear map address), it would be much harder to find the linear map address to attack. An alias with elevated permissions could be used as required, or if it's a one-time RW->RO switch, the mapping could me modified in-place as the granularity wouldn't change.

This would work for new features but probably not for existing features such as the eBPF interpreter.

Quoted 5 lines

Sure.

For eBPF it might be possible to rework the code to support using separate aliases, but that's probably not going to be easy and that probably works against some performance requirement. :/

Ah no, you are correct, I misread how the code was working. The eBPF code does use vmalloc so that can easily be fixed up.

I think your suggestion of either using vmalloc or a special static section is the best recommendation. If anyone really thinks they need to change any other memory they can make a proposal.

Sorry for the confusion.

Quoted 16 lines · Laura Abbott

Quoted 15 lines · Mark Rutland

Hi all,

This series reworks the arm64 early page table code, in order to:

(a) Avoid issues with potentially-conflicting TTBR1 TLB entries (as raised in Jeremy's thread [1]). This can happen when splitting/merging sections or contiguous ranges, and per a pessimistic reading of the ARM ARM may happen for changes to other fields in translation table entries.

(b) Allow for more complex page table creation early on, with tables created with fine-grained permissions as early as possible. In the cases where we currently use fine-grained permissions (e.g. DEBUG_RODATA and marking .init as non-executable), this is required for the same reasons as (a), as we must ensure that changes to page tables do not split/merge sections or contiguous regions for memory in active use.

[...]

Quoted 10 lines

Quoted 9 lines

There are still opportunities for improvement:

* BUG() when splitting sections or creating overlapping entries in create_mapping, as these both indicate serious bugs in kernel page table creation.

This will require rework to the EFI runtime services pagetable creation, as for >4K page kernels EFI memory descriptors may share pages (and currently such overlap is assumed to be benign).

Quoted 3 lines

Given the split_{pmd,pud} were added for DEBUG_RODATA, is there any reason those can't be dropped now since it sounds like the EFI problem is for overlapping entries and not splitting?

Good point. I think they can be removed.

I'll take a look into that.

Mark Rutland (18):
asm-generic: make __set_fixmap_offset a static inline
arm64: mm: specialise pagetable allocators
arm64: mm: place empty_zero_page in bss
arm64: unify idmap removal
arm64: unmap idmap earlier
arm64: add function to install the idmap
arm64: mm: add code to safely replace TTBR1_EL1
arm64: kasan: avoid TLB conflicts
arm64: mm: move pte_* macros
arm64: mm: add functions to walk page tables by PA

arm64: mm: avoid redundant __pa(__va(x))
arm64: mm: add __{pud,pgd}_populate
arm64: mm: add functions to walk tables in fixmap
arm64: mm: use fixmap when creating page tables
arm64: mm: allocate pagetables anywhere
arm64: mm: allow passing a pgdir to alloc_init_*
arm64: ensure _stext and _etext are page-aligned
arm64: mm: create new fine-grained mappings at boot

Quoted 11 lines · Mark Rutland

Mark Rutland (18):
asm-generic: make __set_fixmap_offset a static inline
arm64: mm: specialise pagetable allocators
arm64: mm: place empty_zero_page in bss
arm64: unify idmap removal
arm64: unmap idmap earlier
arm64: add function to install the idmap
arm64: mm: add code to safely replace TTBR1_EL1
arm64: kasan: avoid TLB conflicts
arm64: mm: move pte_* macros
arm64: mm: add functions to walk page tables by PA

Quoted 8 lines

arm64: mm: avoid redundant __pa(__va(x))
arm64: mm: add __{pud,pgd}_populate
arm64: mm: add functions to walk tables in fixmap
arm64: mm: use fixmap when creating page tables
arm64: mm: allocate pagetables anywhere
arm64: mm: allow passing a pgdir to alloc_init_*
arm64: ensure _stext and _etext are page-aligned
arm64: mm: create new fine-grained mappings at boot

The patches look fine (once you fix the issues Laura raised). Thanks for putting them together.

Reviewed-by Catalin Marinas <[email protected]>

I'll queue them sometime after -rc1, in the meantime keep you branch up to date so that Ard and Jeremy can base their patches on top.

Quoted 21 lines · Catalin Marinas

Quoted 11 lines · Mark Rutland

Mark Rutland (18):
asm-generic: make __set_fixmap_offset a static inline
arm64: mm: specialise pagetable allocators
arm64: mm: place empty_zero_page in bss
arm64: unify idmap removal
arm64: unmap idmap earlier
arm64: add function to install the idmap
arm64: mm: add code to safely replace TTBR1_EL1
arm64: kasan: avoid TLB conflicts
arm64: mm: move pte_* macros
arm64: mm: add functions to walk page tables by PA

Quoted 8 lines

arm64: mm: avoid redundant __pa(__va(x))
arm64: mm: add __{pud,pgd}_populate
arm64: mm: add functions to walk tables in fixmap
arm64: mm: use fixmap when creating page tables
arm64: mm: allocate pagetables anywhere
arm64: mm: allow passing a pgdir to alloc_init_*
arm64: ensure _stext and _etext are page-aligned
arm64: mm: create new fine-grained mappings at boot

Quoted 4 lines

The patches look fine (once you fix the issues Laura raised). Thanks for putting them together.

Reviewed-by Catalin Marinas <[email protected]>

Thanks!

I assume that applies to everything even without the suggested split_{pmd,pud} removal [1], for which I'll cook up a follow-up patch.

Quoted 2 lines

I'll queue them sometime after -rc1, in the meantime keep you branch up to date so that Ard and Jeremy can base their patches on top.

Will do.

FWIW I've just updated the branch [2] with said fixes and your Reviewed-by. I won't send out a v3 just yet to give people time to digest this version.

Ard, you'll find when rebasing that the compiler will scream at you due to the p??_fixmap* function renaming. It's fairly mechanical, and if you have vim handy you just need to run:

:%s /\(pgd\|pud\|pmd\|pte\)_fixmap_unmap/\1_clear_fixmap/g
:%s /\(pgd\|pud\|pmd\|pte\)_fixmap/\1_set_fixmap/g

Hi all,

This series reworks the arm64 early page table code, in order to:

(a) Avoid issues with potentially-conflicting TTBR1 TLB entries (as raised in Jeremy's thread [1]). This can happen when splitting/merging sections or contiguous ranges, and per a pessimistic reading of the ARM ARM may happen for changes to other fields in translation table entries.

(b) Allow for more complex page table creation early on, with tables created with fine-grained permissions as early as possible. In the cases where we currently use fine-grained permissions (e.g. DEBUG_RODATA and marking .init as non-executable), this is required for the same reasons as (a), as we must ensure that changes to page tables do not split/merge sections or contiguous regions for memory in active use.

(c) Avoid edge cases where we need to allocate memory before a sufficient proportion of the early linear map is in place to accommodate allocations.

This series:

* Introduces the necessary infrastructure to safely swap TTBR1_EL1 (i.e. without risking conflicting TLB entries being allocated). The arm64 KASAN code is migrated to this.

* Adds helpers to walk page tables by physical address, independent of the linear mapping, and modifies __create_mapping and friends to relying on a new set of FIX_{PGD,PUD,PMD,PTE} to map tables as required for modification.

* Removes the early memblock limit, now that create_mapping does not rely on the early linear map. This solves (c), and allows for (b).

* Generates an entirely new set of kernel page tables with fine-grained (i.e. page-level) permission boundaries, which can then be safely installed. These are created with sufficient granularity such that later changes (currently only fixup_init) will not split/merge sections or contiguous regions, and can follow a break-before-make approach without affecting the rest of the page tables.

There are still opportunities for improvement:

* BUG() when splitting sections or creating overlapping entries in create_mapping, as these both indicate serious bugs in kernel page table creation.

This will require rework to the EFI runtime services pagetable creation, as for >4K page kernels EFI memory descriptors may share pages (and currently such overlap is assumed to be benign).

* Use ROX mappings for the kernel text and rodata when creating the new tables. This avoiding potential conflicts from changes to translation tables, and giving us better protections earlier.

Currently the alternatives patching code relies on being able to use the kernel mapping to update the text. We cannot rely on any text which itself may be patched, and updates may straddle page boundaries, so this is non-trivial.

* Clean up usage of swapper_pg_dir so we can switch to the new tables without having to reuse the existing pgd. This will allow us to free the original pgd (i.e. we can free all the initial tables in one go).

Any and all feedback is welcome.

This series is based on today's arm64 [2] for-next/core branch (commit c9cd0ed925c0b927), and this version is tagged as arm64-pagetable-rework-20160104 while the latest version should be in the unstable branch arm64/pagetable-rework in my git repo [3].

Since v1 [4] (tagged arm64-pagetable-rework-20151209):
* Drop patches taken into the arm64 tree.
* Rebase to arm64 for-next/core.
* Copy early KASAN tables.
* Fix KASAN pgd manipulation.
* Specialise allocators for page tables, in function and naming.
* Update comments.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-November/386178.html
[2] git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git
[3] git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git
[4] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-December/392292.html

Mark Rutland (18):
asm-generic: make __set_fixmap_offset a static inline
arm64: mm: specialise pagetable allocators
arm64: mm: place empty_zero_page in bss
arm64: unify idmap removal
arm64: unmap idmap earlier
arm64: add function to install the idmap
arm64: mm: add code to safely replace TTBR1_EL1
arm64: kasan: avoid TLB conflicts
arm64: mm: move pte_* macros
arm64: mm: add functions to walk page tables by PA
arm64: mm: avoid redundant __pa(__va(x))
arm64: mm: add __{pud,pgd}_populate
arm64: mm: add functions to walk tables in fixmap
arm64: mm: use fixmap when creating page tables
arm64: mm: allocate pagetables anywhere
arm64: mm: allow passing a pgdir to alloc_init_*
arm64: ensure _stext and _etext are page-aligned
arm64: mm: create new fine-grained mappings at boot

 arch/arm64/include/asm/fixmap.h      |  10 ++
 arch/arm64/include/asm/kasan.h       |   3 +
 arch/arm64/include/asm/mmu_context.h |  63 ++++++-
 arch/arm64/include/asm/pgalloc.h     |  26 ++-
 arch/arm64/include/asm/pgtable.h     |  87 +++++++---
 arch/arm64/kernel/head.S             |   1 +
 arch/arm64/kernel/setup.c            |   7 +
 arch/arm64/kernel/smp.c              |   4 +-
 arch/arm64/kernel/suspend.c          |  20 +--
 arch/arm64/kernel/vmlinux.lds.S      |   5 +-
 arch/arm64/mm/kasan_init.c           |  32 ++--
 arch/arm64/mm/mmu.c                  | 311 ++++++++++++++++++-----------------
 arch/arm64/mm/proc.S                 |  27 +++
 include/asm-generic/fixmap.h         |  14 +-
 14 files changed, 381 insertions(+), 229 deletions(-)

--
1.9.1