Skip to contentSkip to conversation

Use global pages with PTI

PATCH Superseded Dave Hansen 6 participants 20 messages Apr 4, 2018 – Apr 5, 2018

Subsystems (5)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Dave Hansen

author maintainer
#0 lore raw

v4 cover Use global pages with PTI

Changes from v3:
* Fix whitespace issue noticed by willy
* Clarify comments about X86_FEATURE_PGE checks
* Clarify commit message around the necessity of _PAGE_GLOBAL filtering when CR4.PGE=0 or PGE is unsupported.

Changes from v2:

* Add performance numbers to changelogs
* Fix compile error resulting from use of x86-specific __default_kernel_pte_mask in arch-generic mm/early_ioremap.c
* Delay kernel text cloning until after we are done messing with it (patch 11).
* Blacklist K8 explicitly from mapping all kernel text as global (this should never happen because K8 does not use pti when pti=auto, but we on the safe side). (patch 11)

--

The later versions of the KAISER patches (pre-PTI) allowed the user/kernel shared areas to be GLOBAL. The thought was that this would reduce the TLB overhead of keeping two copies of these mappings.

During the switch over to PTI, we seem to have lost our ability to have GLOBAL mappings. This adds them back.

To measure the benefits of this, I took a modern Atom system without PCIDs and ran a microbenchmark[1] (higher is better):

No Global Lines (baseline ): 6077741 lseeks/sec 88 Global Lines (kern entry): 7528609 lseeks/sec (+23.9%) 94 Global Lines (all ktext ): 8433111 lseeks/sec (+38.8%)

On a modern Skylake desktop with PCIDs, the benefits are tangible, but not huge:

No Global pages (baseline): 15783951 lseeks/sec
28 Global pages (this set): 16054688 lseeks/sec

                             +270737 lseeks/sec (+1.71%)

I also double-checked with a kernel compile on the Skylake system (lower is better):

No Global pages (baseline): 186.951 seconds time elapsed  ( +-  0.35% )
28 Global pages (this set): 185.756 seconds time elapsed  ( +-  0.09% )
                             -1.195 seconds (-0.64%)

1. https://github.com/antonblanchard/will-it-scale/blob/master/tests/lseek1.c

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>
author maintainer
#1 lore raw

PATCH v4 1/11 x86/mm: factor out pageattr _PAGE_GLOBAL setting

From: Dave Hansen <[email protected]>

The pageattr code has a pattern repeated where it sets _PAGE_GLOBAL for present PTEs but clears it for non-present PTEs. The intention is to keep _PAGE_GLOBAL from getting confused with _PAGE_PROTNONE since _PAGE_GLOBAL is for present PTEs and _PAGE_PROTNONE is for non-present

But, this pattern makes no sense. Effectively, it says, if you use the pageattr code, always set _PAGE_GLOBAL when _PAGE_PRESENT. canon_pgprot() will clear it if unsupported (because it masks the value with __supported_pte_mask) but we *always* set it. Even if canon_pgprot() did not filter _PAGE_GLOBAL, it would be OK. _PAGE_GLOBAL is ignored when CR4.PGE=0 by the hardware.

This unconditional setting of _PAGE_GLOBAL is a problem when we have PTI and non-PTI and we want some areas to have _PAGE_GLOBAL and some not.

This updated version of the code says:
1. Clear _PAGE_GLOBAL when !_PAGE_PRESENT
2. Never set _PAGE_GLOBAL implicitly
3. Allow _PAGE_GLOBAL to be in cpa.set_mask
4. Allow _PAGE_GLOBAL to be inherited from previous PTE

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/mm/pageattr.c |   66 ++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 43 deletions(-)
arch/x86/mm/pageattr.c renamed from arch/x86/mm/pageattr.c~kpti-centralize-global-setting +23 -43
diff -puN arch/x86/mm/pageattr.c~kpti-centralize-global-setting arch/x86/mm/pageattr.c
@@ -512,6 +512,23 @@ static void __set_pmd_pte(pte_t *kpte, u
 #endif
 }
 
+static pgprot_t pgprot_clear_protnone_bits(pgprot_t prot)
+{
+	/*
+	 * _PAGE_GLOBAL means "global page" for present PTEs.
+	 * But, it is also used to indicate _PAGE_PROTNONE
+	 * for non-present PTEs.
+	 *
+	 * This ensures that a _PAGE_GLOBAL PTE going from
+	 * present to non-present is not confused as
+	 * _PAGE_PROTNONE.
+	 */
+	if (!(pgprot_val(prot) & _PAGE_PRESENT))
+		pgprot_val(prot) &= ~_PAGE_GLOBAL;
+
+	return prot;
+}
+
 static int
 try_preserve_large_page(pte_t *kpte, unsigned long address,
 			struct cpa_data *cpa)
@@ -577,18 +594,11 @@ try_preserve_large_page(pte_t *kpte, uns
 	 * different bit positions in the two formats.
 	 */
 	req_prot = pgprot_4k_2_large(req_prot);
-
-	/*
-	 * Set the PSE and GLOBAL flags only if the PRESENT flag is
-	 * set otherwise pmd_present/pmd_huge will return true even on
-	 * a non present pmd. The canon_pgprot will clear _PAGE_GLOBAL
-	 * for the ancient hardware that doesn't support it.
-	 */
+	req_prot = pgprot_clear_protnone_bits(req_prot);
 	if (pgprot_val(req_prot) & _PAGE_PRESENT)
-		pgprot_val(req_prot) |= _PAGE_PSE | _PAGE_GLOBAL;
+		pgprot_val(req_prot) |= _PAGE_PSE;
 	else
-		pgprot_val(req_prot) &= ~(_PAGE_PSE | _PAGE_GLOBAL);
-
+		pgprot_val(req_prot) &= ~_PAGE_PSE;
 	req_prot = canon_pgprot(req_prot);
 
 	/*
@@ -698,16 +708,7 @@ __split_large_page(struct cpa_data *cpa,
 		return 1;
 	}
 
-	/*
-	 * Set the GLOBAL flags only if the PRESENT flag is set
-	 * otherwise pmd/pte_present will return true even on a non
-	 * present pmd/pte. The canon_pgprot will clear _PAGE_GLOBAL
-	 * for the ancient hardware that doesn't support it.
-	 */
-	if (pgprot_val(ref_prot) & _PAGE_PRESENT)
-		pgprot_val(ref_prot) |= _PAGE_GLOBAL;
-	else
-		pgprot_val(ref_prot) &= ~_PAGE_GLOBAL;
+	ref_prot = pgprot_clear_protnone_bits(ref_prot);
 
 	/*
 	 * Get the target pfn from the original entry:
@@ -930,18 +931,7 @@ static void populate_pte(struct cpa_data
 
 	pte = pte_offset_kernel(pmd, start);
 
-	/*
-	 * Set the GLOBAL flags only if the PRESENT flag is
-	 * set otherwise pte_present will return true even on
-	 * a non present pte. The canon_pgprot will clear
-	 * _PAGE_GLOBAL for the ancient hardware that doesn't
-	 * support it.
-	 */
-	if (pgprot_val(pgprot) & _PAGE_PRESENT)
-		pgprot_val(pgprot) |= _PAGE_GLOBAL;
-	else
-		pgprot_val(pgprot) &= ~_PAGE_GLOBAL;
-
+	pgprot = pgprot_clear_protnone_bits(pgprot);
 	pgprot = canon_pgprot(pgprot);
 
 	while (num_pages-- && start < end) {
@@ -1234,17 +1224,7 @@ repeat:
 
 		new_prot = static_protections(new_prot, address, pfn);
 
-		/*
-		 * Set the GLOBAL flags only if the PRESENT flag is
-		 * set otherwise pte_present will return true even on
-		 * a non present pte. The canon_pgprot will clear
-		 * _PAGE_GLOBAL for the ancient hardware that doesn't
-		 * support it.
-		 */
-		if (pgprot_val(new_prot) & _PAGE_PRESENT)
-			pgprot_val(new_prot) |= _PAGE_GLOBAL;
-		else
-			pgprot_val(new_prot) &= ~_PAGE_GLOBAL;
+		new_prot = pgprot_clear_protnone_bits(new_prot);
 
 		/*
 		 * We need to keep the pfn from the existing PTE,

_

author maintainer
#2 lore raw

PATCH v4 2/11 x86/mm: undo double _PAGE_PSE clearing

In reply to Dave Hansen

From: Dave Hansen <[email protected]>

When clearing _PAGE_PRESENT on a huge page, we need to be careful to also clear _PAGE_PSE, otherwise it might still get confused for a valid large page table entry.

We do that near the spot where we *set* _PAGE_PSE. That's fine, but it's unnecessary. pgprot_large_2_4k() already did it.

BTW, I also noticed that pgprot_large_2_4k() and pgprot_4k_2_large() are not symmetric. pgprot_large_2_4k() clears _PAGE_PSE (because it is aliased to _PAGE_PAT) but pgprot_4k_2_large() does not put _PAGE_PSE back. Bummer.

Also, add some comments and change "promote" to "move". "Promote" seems an odd word to move when we are logically moving a bit to a lower bit position. Also add an extra line return to make it clear to which line the comment applies.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/mm/pageattr.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
arch/x86/mm/pageattr.c renamed from arch/x86/mm/pageattr.c~kpti-undo-double-_PAGE_PSE-clear +6 -3
diff -puN arch/x86/mm/pageattr.c~kpti-undo-double-_PAGE_PSE-clear arch/x86/mm/pageattr.c
@@ -583,6 +583,7 @@ try_preserve_large_page(pte_t *kpte, uns
 	 * up accordingly.
 	 */
 	old_pte = *kpte;
+	/* Clear PSE (aka _PAGE_PAT) and move PAT bit to correct position */
 	req_prot = pgprot_large_2_4k(old_prot);
 
 	pgprot_val(req_prot) &= ~pgprot_val(cpa->mask_clr);
@@ -597,8 +598,6 @@ try_preserve_large_page(pte_t *kpte, uns
 	req_prot = pgprot_clear_protnone_bits(req_prot);
 	if (pgprot_val(req_prot) & _PAGE_PRESENT)
 		pgprot_val(req_prot) |= _PAGE_PSE;
-	else
-		pgprot_val(req_prot) &= ~_PAGE_PSE;
 	req_prot = canon_pgprot(req_prot);
 
 	/*
@@ -684,8 +683,12 @@ __split_large_page(struct cpa_data *cpa,
 	switch (level) {
 	case PG_LEVEL_2M:
 		ref_prot = pmd_pgprot(*(pmd_t *)kpte);
-		/* clear PSE and promote PAT bit to correct position */
+		/*
+		 * Clear PSE (aka _PAGE_PAT) and move
+		 * PAT bit to correct position.
+		 */
 		ref_prot = pgprot_large_2_4k(ref_prot);
+
 		ref_pfn = pmd_pfn(*(pmd_t *)kpte);
 		break;
 

_

author maintainer
#3 lore raw

PATCH v4 3/11 x86/mm: introduce "default" kernel PTE mask

In reply to Dave Hansen

From: Dave Hansen <[email protected]>

The __PAGE_KERNEL_* page permissions are "raw". They contain bits that may or may not be supported on the current processor. They need to be filtered by a mask (currently __supported_pte_mask) to turn them into a value that we can actually set in a PTE.

These __PAGE_KERNEL_* values all contain _PAGE_GLOBAL. But, with PTI, we want to be able to support _PAGE_GLOBAL (have the bit set in __supported_pte_mask) but not have it appear in any of these masks by default.

This patch creates a new mask, __default_kernel_pte_mask, and applies it when creating all of the PAGE_KERNEL_* masks. This makes PAGE_KERNEL_* safe to use anywhere (they only contain supported bits). It also ensures that PAGE_KERNEL_* contains _PAGE_GLOBAL on PTI=n kernels but clears _PAGE_GLOBAL when PTI=y.

We also make __default_kernel_pte_mask a non-GPL exported symbol because there are plenty of driver-available interfaces that take PAGE_KERNEL_* permissions.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/include/asm/pgtable_types.h |   27 +++++++++++++++------------
 b/arch/x86/mm/init.c                   |    6 ++++++
 b/arch/x86/mm/init_32.c                |    8 +++++++-
 b/arch/x86/mm/init_64.c                |    5 +++++
 4 files changed, 33 insertions(+), 13 deletions(-)
arch/x86/include/asm/pgtable_types.h renamed from arch/x86/include/asm/pgtable_types.h~KERN-pgprot-default +15 -12
diff -puN arch/x86/include/asm/pgtable_types.h~KERN-pgprot-default arch/x86/include/asm/pgtable_types.h
@@ -196,19 +196,21 @@ enum page_cache_mode {
 #define __PAGE_KERNEL_NOENC	(__PAGE_KERNEL)
 #define __PAGE_KERNEL_NOENC_WP	(__PAGE_KERNEL_WP)
 
-#define PAGE_KERNEL		__pgprot(__PAGE_KERNEL | _PAGE_ENC)
-#define PAGE_KERNEL_NOENC	__pgprot(__PAGE_KERNEL)
-#define PAGE_KERNEL_RO		__pgprot(__PAGE_KERNEL_RO | _PAGE_ENC)
-#define PAGE_KERNEL_EXEC	__pgprot(__PAGE_KERNEL_EXEC | _PAGE_ENC)
-#define PAGE_KERNEL_EXEC_NOENC	__pgprot(__PAGE_KERNEL_EXEC)
-#define PAGE_KERNEL_RX		__pgprot(__PAGE_KERNEL_RX | _PAGE_ENC)
-#define PAGE_KERNEL_NOCACHE	__pgprot(__PAGE_KERNEL_NOCACHE | _PAGE_ENC)
-#define PAGE_KERNEL_LARGE	__pgprot(__PAGE_KERNEL_LARGE | _PAGE_ENC)
-#define PAGE_KERNEL_LARGE_EXEC	__pgprot(__PAGE_KERNEL_LARGE_EXEC | _PAGE_ENC)
-#define PAGE_KERNEL_VVAR	__pgprot(__PAGE_KERNEL_VVAR | _PAGE_ENC)
+#define default_pgprot(x)	__pgprot((x) & __default_kernel_pte_mask)
 
-#define PAGE_KERNEL_IO		__pgprot(__PAGE_KERNEL_IO)
-#define PAGE_KERNEL_IO_NOCACHE	__pgprot(__PAGE_KERNEL_IO_NOCACHE)
+#define PAGE_KERNEL		default_pgprot(__PAGE_KERNEL | _PAGE_ENC)
+#define PAGE_KERNEL_NOENC	default_pgprot(__PAGE_KERNEL)
+#define PAGE_KERNEL_RO		default_pgprot(__PAGE_KERNEL_RO | _PAGE_ENC)
+#define PAGE_KERNEL_EXEC	default_pgprot(__PAGE_KERNEL_EXEC | _PAGE_ENC)
+#define PAGE_KERNEL_EXEC_NOENC	default_pgprot(__PAGE_KERNEL_EXEC)
+#define PAGE_KERNEL_RX		default_pgprot(__PAGE_KERNEL_RX | _PAGE_ENC)
+#define PAGE_KERNEL_NOCACHE	default_pgprot(__PAGE_KERNEL_NOCACHE | _PAGE_ENC)
+#define PAGE_KERNEL_LARGE	default_pgprot(__PAGE_KERNEL_LARGE | _PAGE_ENC)
+#define PAGE_KERNEL_LARGE_EXEC	default_pgprot(__PAGE_KERNEL_LARGE_EXEC | _PAGE_ENC)
+#define PAGE_KERNEL_VVAR	default_pgprot(__PAGE_KERNEL_VVAR | _PAGE_ENC)
+
+#define PAGE_KERNEL_IO		default_pgprot(__PAGE_KERNEL_IO)
+#define PAGE_KERNEL_IO_NOCACHE	default_pgprot(__PAGE_KERNEL_IO_NOCACHE)
 
 #endif	/* __ASSEMBLY__ */
 
@@ -483,6 +485,7 @@ static inline pgprot_t pgprot_large_2_4k
 typedef struct page *pgtable_t;
 
 extern pteval_t __supported_pte_mask;
+extern pteval_t __default_kernel_pte_mask;
 extern void set_nx(void);
 extern int nx_enabled;
 
arch/x86/mm/init_32.c renamed from arch/x86/mm/init_32.c~KERN-pgprot-default +7 -1
diff -puN arch/x86/mm/init_32.c~KERN-pgprot-default arch/x86/mm/init_32.c
@@ -558,8 +558,14 @@ static void __init pagetable_init(void)
 	permanent_kmaps_init(pgd_base);
 }
 
-pteval_t __supported_pte_mask __read_mostly = ~(_PAGE_NX | _PAGE_GLOBAL);
+#define DEFAULT_PTE_MASK ~(_PAGE_NX | _PAGE_GLOBAL)
+/* Bits supported by the hardware: */
+pteval_t __supported_pte_mask __read_mostly = DEFAULT_PTE_MASK;
+/* Bits allowed in normal kernel mappings: */
+pteval_t __default_kernel_pte_mask __read_mostly = DEFAULT_PTE_MASK;
 EXPORT_SYMBOL_GPL(__supported_pte_mask);
+/* Used in PAGE_KERNEL_* macros which are reasonably used out-of-tree: */
+EXPORT_SYMBOL(__default_kernel_pte_mask);
 
 /* user-defined highmem size */
 static unsigned int highmem_pages = -1;
arch/x86/mm/init_64.c renamed from arch/x86/mm/init_64.c~KERN-pgprot-default +5 -0
diff -puN arch/x86/mm/init_64.c~KERN-pgprot-default arch/x86/mm/init_64.c
@@ -65,8 +65,13 @@
  * around without checking the pgd every time.
  */
 
+/* Bits supported by the hardware: */
 pteval_t __supported_pte_mask __read_mostly = ~0;
+/* Bits allowed in normal kernel mappings: */
+pteval_t __default_kernel_pte_mask __read_mostly = ~0;
 EXPORT_SYMBOL_GPL(__supported_pte_mask);
+/* Used in PAGE_KERNEL_* macros which are reasonably used out-of-tree: */
+EXPORT_SYMBOL(__default_kernel_pte_mask);
 
 int force_personality32;
 
arch/x86/mm/init.c renamed from arch/x86/mm/init.c~KERN-pgprot-default +6 -0
diff -puN arch/x86/mm/init.c~KERN-pgprot-default arch/x86/mm/init.c
@@ -190,6 +190,12 @@ static void __init probe_page_size_mask(
 		enable_global_pages();
 	}
 
+	/* By the default is everything supported: */
+	__default_kernel_pte_mask = __supported_pte_mask;
+	/* Except when with PTI where the kernel is mostly non-Global: */
+	if (cpu_feature_enabled(X86_FEATURE_PTI))
+		__default_kernel_pte_mask &= ~_PAGE_GLOBAL;
+
 	/* Enable 1 GB linear kernel mappings if available: */
 	if (direct_gbpages && boot_cpu_has(X86_FEATURE_GBPAGES)) {
 		printk(KERN_INFO "Using GB pages for direct mapping\n");

_

author maintainer
#4 lore raw

PATCH v4 4/11 x86/espfix: document use of _PAGE_GLOBAL

In reply to Dave Hansen

From: Dave Hansen <[email protected]>

The "normal" kernel page table creation mechanisms using PAGE_KERNEL_* page protections will never set _PAGE_GLOBAL with PTI. The few places in the kernel that always want _PAGE_GLOBAL must avoid using PAGE_KERNEL_*.

Document that we want it here and its use is not accidental.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/kernel/espfix_64.c |    4 ++++
 1 file changed, 4 insertions(+)
arch/x86/kernel/espfix_64.c renamed from arch/x86/kernel/espfix_64.c~espfix-use-kern-defaults-not-supported +4 -0
diff -puN arch/x86/kernel/espfix_64.c~espfix-use-kern-defaults-not-supported arch/x86/kernel/espfix_64.c
@@ -195,6 +195,10 @@ void init_espfix_ap(int cpu)
 
 	pte_p = pte_offset_kernel(&pmd, addr);
 	stack_page = page_address(alloc_pages_node(node, GFP_KERNEL, 0));
+	/*
+	 * __PAGE_KERNEL_* includes _PAGE_GLOBAL, which we want since
+	 * this is mapped to userspace.
+	 */
 	pte = __pte(__pa(stack_page) | ((__PAGE_KERNEL_RO | _PAGE_ENC) & ptemask));
 	for (n = 0; n < ESPFIX_PTE_CLONES; n++)
 		set_pte(&pte_p[n*PTE_STRIDE], pte);

_

author maintainer
#5 lore raw

PATCH v4 5/11 x86/mm: do not auto-massage page protections

In reply to Dave Hansen

From: Dave Hansen <[email protected]>

A PTE is constructed from a physical address and a pgprotval_t. __PAGE_KERNEL, for instance, is a pgprot_t and must be converted into a pgprotval_t before it can be used to create a PTE. This is done implicitly within functions like pfn_pte() by massage_pgprot().

However, this makes it very challenging to set bits (and keep them set) if your bit is being filtered out by massage_pgprot().

This moves the bit filtering out of pfn_pte() and friends. For users of PAGE_KERNEL*, filtering will be done automatically inside those macros but for users of __PAGE_KERNEL*, they need to do their own filtering now.

Note that we also just move pfn_pte/pmd/pud() over to check_pgprot() instead of massage_pgprot(). This way, we still *look* for unsupported bits and properly warn about them if we find them. This might happen if an unfiltered __PAGE_KERNEL* value was passed in, for instance.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/include/asm/pgtable.h |   27 ++++++++++++++++++++++-----
 b/arch/x86/kernel/head64.c       |    2 ++
 b/arch/x86/kernel/ldt.c          |    6 +++++-
 b/arch/x86/mm/ident_map.c        |    3 +++
 b/arch/x86/mm/iomap_32.c         |    6 ++++++
 b/arch/x86/mm/ioremap.c          |    3 +++
 b/arch/x86/mm/kasan_init_64.c    |   14 +++++++++++++-
 b/arch/x86/mm/pgtable.c          |    3 +++
 b/arch/x86/power/hibernate_64.c  |   20 +++++++++++++++-----
 9 files changed, 72 insertions(+), 12 deletions(-)
arch/x86/include/asm/pgtable.h renamed from arch/x86/include/asm/pgtable.h~x86-no-auto-massage +22 -5
diff -puN arch/x86/include/asm/pgtable.h~x86-no-auto-massage arch/x86/include/asm/pgtable.h
@@ -526,22 +526,39 @@ static inline pgprotval_t massage_pgprot
 	return protval;
 }
 
+static inline pgprotval_t check_pgprot(pgprot_t pgprot)
+{
+	pgprotval_t massaged_val = massage_pgprot(pgprot);
+
+	/* mmdebug.h can not be included here because of dependencies */
+#ifdef CONFIG_DEBUG_VM
+	WARN_ONCE(pgprot_val(pgprot) != massaged_val,
+		  "attempted to set unsupported pgprot: %016lx "
+		  "bits: %016lx supported: %016lx\n",
+		  pgprot_val(pgprot),
+		  pgprot_val(pgprot) ^ massaged_val,
+		  __supported_pte_mask);
+#endif
+
+	return massaged_val;
+}
+
 static inline pte_t pfn_pte(unsigned long page_nr, pgprot_t pgprot)
 {
 	return __pte(((phys_addr_t)page_nr << PAGE_SHIFT) |
-		     massage_pgprot(pgprot));
+		     check_pgprot(pgprot));
 }
 
 static inline pmd_t pfn_pmd(unsigned long page_nr, pgprot_t pgprot)
 {
 	return __pmd(((phys_addr_t)page_nr << PAGE_SHIFT) |
-		     massage_pgprot(pgprot));
+		     check_pgprot(pgprot));
 }
 
 static inline pud_t pfn_pud(unsigned long page_nr, pgprot_t pgprot)
 {
 	return __pud(((phys_addr_t)page_nr << PAGE_SHIFT) |
-		     massage_pgprot(pgprot));
+		     check_pgprot(pgprot));
 }
 
 static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
@@ -553,7 +570,7 @@ static inline pte_t pte_modify(pte_t pte
 	 * the newprot (if present):
 	 */
 	val &= _PAGE_CHG_MASK;
-	val |= massage_pgprot(newprot) & ~_PAGE_CHG_MASK;
+	val |= check_pgprot(newprot) & ~_PAGE_CHG_MASK;
 
 	return __pte(val);
 }
@@ -563,7 +580,7 @@ static inline pmd_t pmd_modify(pmd_t pmd
 	pmdval_t val = pmd_val(pmd);
 
 	val &= _HPAGE_CHG_MASK;
-	val |= massage_pgprot(newprot) & ~_HPAGE_CHG_MASK;
+	val |= check_pgprot(newprot) & ~_HPAGE_CHG_MASK;
 
 	return __pmd(val);
 }
arch/x86/kernel/head64.c renamed from arch/x86/kernel/head64.c~x86-no-auto-massage +2 -0
diff -puN arch/x86/kernel/head64.c~x86-no-auto-massage arch/x86/kernel/head64.c
@@ -195,6 +195,8 @@ unsigned long __head __startup_64(unsign
 	pud[i + 1] = (pudval_t)pmd + pgtable_flags;
 
 	pmd_entry = __PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL;
+	/* Filter out unsupported __PAGE_KERNEL_* bits: */
+	pmd_entry &= __supported_pte_mask;
 	pmd_entry += sme_get_me_mask();
 	pmd_entry +=  physaddr;
 
arch/x86/kernel/ldt.c renamed from arch/x86/kernel/ldt.c~x86-no-auto-massage +5 -1
diff -puN arch/x86/kernel/ldt.c~x86-no-auto-massage arch/x86/kernel/ldt.c
@@ -145,6 +145,7 @@ map_ldt_struct(struct mm_struct *mm, str
 		unsigned long offset = i << PAGE_SHIFT;
 		const void *src = (char *)ldt->entries + offset;
 		unsigned long pfn;
+		pgprot_t pte_prot;
 		pte_t pte, *ptep;
 
 		va = (unsigned long)ldt_slot_va(slot) + offset;
@@ -163,7 +164,10 @@ map_ldt_struct(struct mm_struct *mm, str
 		 * target via some kernel interface which misses a
 		 * permission check.
 		 */
-		pte = pfn_pte(pfn, __pgprot(__PAGE_KERNEL_RO & ~_PAGE_GLOBAL));
+	       	pte_prot = __pgprot(__PAGE_KERNEL_RO & ~_PAGE_GLOBAL);
+		/* Filter out unsuppored __PAGE_KERNEL* bits: */
+		pgprot_val(pte_prot) |= __supported_pte_mask;
+		pte = pfn_pte(pfn, pte_prot);
 		set_pte_at(mm, va, ptep, pte);
 		pte_unmap_unlock(ptep, ptl);
 	}
arch/x86/mm/ident_map.c renamed from arch/x86/mm/ident_map.c~x86-no-auto-massage +3 -0
diff -puN arch/x86/mm/ident_map.c~x86-no-auto-massage arch/x86/mm/ident_map.c
@@ -98,6 +98,9 @@ int kernel_ident_mapping_init(struct x86
 	if (!info->kernpg_flag)
 		info->kernpg_flag = _KERNPG_TABLE;
 
+	/* Filter out unsupported __PAGE_KERNEL_* bits: */
+	info->kernpg_flag &= __default_kernel_pte_mask;
+
 	for (; addr < end; addr = next) {
 		pgd_t *pgd = pgd_page + pgd_index(addr);
 		p4d_t *p4d;
arch/x86/mm/iomap_32.c renamed from arch/x86/mm/iomap_32.c~x86-no-auto-massage +6 -0
diff -puN arch/x86/mm/iomap_32.c~x86-no-auto-massage arch/x86/mm/iomap_32.c
@@ -44,6 +44,9 @@ int iomap_create_wc(resource_size_t base
 		return ret;
 
 	*prot = __pgprot(__PAGE_KERNEL | cachemode2protval(pcm));
+	/* Filter out unsupported __PAGE_KERNEL* bits: */
+	pgprot_val(*prot) &= __default_kernel_pte_mask;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iomap_create_wc);
@@ -88,6 +91,9 @@ iomap_atomic_prot_pfn(unsigned long pfn,
 		prot = __pgprot(__PAGE_KERNEL |
 				cachemode2protval(_PAGE_CACHE_MODE_UC_MINUS));
 
+	/* Filter out unsupported __PAGE_KERNEL* bits: */
+	pgprot_val(prot) &= __default_kernel_pte_mask;
+
 	return (void __force __iomem *) kmap_atomic_prot_pfn(pfn, prot);
 }
 EXPORT_SYMBOL_GPL(iomap_atomic_prot_pfn);
arch/x86/mm/ioremap.c renamed from arch/x86/mm/ioremap.c~x86-no-auto-massage +3 -0
diff -puN arch/x86/mm/ioremap.c~x86-no-auto-massage arch/x86/mm/ioremap.c
@@ -816,6 +816,9 @@ void __init __early_set_fixmap(enum fixe
 	}
 	pte = early_ioremap_pte(addr);
 
+        /* Sanitize 'prot' against any unsupported bits: */
+        pgprot_val(flags) &= __default_kernel_pte_mask;
+
 	if (pgprot_val(flags))
 		set_pte(pte, pfn_pte(phys >> PAGE_SHIFT, flags));
 	else
arch/x86/mm/kasan_init_64.c renamed from arch/x86/mm/kasan_init_64.c~x86-no-auto-massage +13 -1
diff -puN arch/x86/mm/kasan_init_64.c~x86-no-auto-massage arch/x86/mm/kasan_init_64.c
@@ -269,6 +269,12 @@ void __init kasan_early_init(void)
 	pudval_t pud_val = __pa_nodebug(kasan_zero_pmd) | _KERNPG_TABLE;
 	p4dval_t p4d_val = __pa_nodebug(kasan_zero_pud) | _KERNPG_TABLE;
 
+	/* Mask out unsupported __PAGE_KERNEL bits: */
+	pte_val &= __default_kernel_pte_mask;
+	pmd_val &= __default_kernel_pte_mask;
+	pud_val &= __default_kernel_pte_mask;
+	p4d_val &= __default_kernel_pte_mask;
+
 	for (i = 0; i < PTRS_PER_PTE; i++)
 		kasan_zero_pte[i] = __pte(pte_val);
 
@@ -371,7 +377,13 @@ void __init kasan_init(void)
 	 */
 	memset(kasan_zero_page, 0, PAGE_SIZE);
 	for (i = 0; i < PTRS_PER_PTE; i++) {
-		pte_t pte = __pte(__pa(kasan_zero_page) | __PAGE_KERNEL_RO | _PAGE_ENC);
+		pte_t pte;
+		pgprot_t prot;
+
+		prot = __pgprot(__PAGE_KERNEL_RO | _PAGE_ENC);
+		pgprot_val(prot) &= __default_kernel_pte_mask;
+
+		pte = __pte(__pa(kasan_zero_page) | pgprot_val(prot));
 		set_pte(&kasan_zero_pte[i], pte);
 	}
 	/* Flush TLBs again to be sure that write protection applied. */
arch/x86/mm/pgtable.c renamed from arch/x86/mm/pgtable.c~x86-no-auto-massage +3 -0
diff -puN arch/x86/mm/pgtable.c~x86-no-auto-massage arch/x86/mm/pgtable.c
@@ -583,6 +583,9 @@ void __native_set_fixmap(enum fixed_addr
 void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
 		       pgprot_t flags)
 {
+	/* Sanitize 'prot' against any unsupported bits: */
+	pgprot_val(flags) &= __default_kernel_pte_mask;
+
 	__native_set_fixmap(idx, pfn_pte(phys >> PAGE_SHIFT, flags));
 }
 
arch/x86/power/hibernate_64.c renamed from arch/x86/power/hibernate_64.c~x86-no-auto-massage +15 -5
diff -puN arch/x86/power/hibernate_64.c~x86-no-auto-massage arch/x86/power/hibernate_64.c
@@ -51,6 +51,12 @@ static int set_up_temporary_text_mapping
 	pmd_t *pmd;
 	pud_t *pud;
 	p4d_t *p4d = NULL;
+	pgprot_t pgtable_prot = __pgprot(_KERNPG_TABLE);
+	pgprot_t pmd_text_prot = __pgprot(__PAGE_KERNEL_LARGE_EXEC);
+
+	/* Filter out unsupported __PAGE_KERNEL* bits: */
+	pgprot_val(pmd_text_prot) &= __default_kernel_pte_mask;
+	pgprot_val(pgtable_prot)  &= __default_kernel_pte_mask;
 
 	/*
 	 * The new mapping only has to cover the page containing the image
@@ -81,15 +87,19 @@ static int set_up_temporary_text_mapping
 		return -ENOMEM;
 
 	set_pmd(pmd + pmd_index(restore_jump_address),
-		__pmd((jump_address_phys & PMD_MASK) | __PAGE_KERNEL_LARGE_EXEC));
+		__pmd((jump_address_phys & PMD_MASK) | pgprot_val(pmd_text_prot)));
 	set_pud(pud + pud_index(restore_jump_address),
-		__pud(__pa(pmd) | _KERNPG_TABLE));
+		__pud(__pa(pmd) | pgprot_val(pgtable_prot)));
 	if (p4d) {
-		set_p4d(p4d + p4d_index(restore_jump_address), __p4d(__pa(pud) | _KERNPG_TABLE));
-		set_pgd(pgd + pgd_index(restore_jump_address), __pgd(__pa(p4d) | _KERNPG_TABLE));
+		p4d_t new_p4d = __p4d(__pa(pud) | pgprot_val(pgtable_prot));
+		pgd_t new_pgd = __pgd(__pa(p4d) | pgprot_val(pgtable_prot));
+
+		set_p4d(p4d + p4d_index(restore_jump_address), new_p4d);
+		set_pgd(pgd + pgd_index(restore_jump_address), new_pgd);
 	} else {
 		/* No p4d for 4-level paging: point the pgd to the pud page table */
-		set_pgd(pgd + pgd_index(restore_jump_address), __pgd(__pa(pud) | _KERNPG_TABLE));
+		pgd_t new_pgd = __pgd(__pa(p4d) | pgprot_val(pgtable_prot));
+		set_pgd(pgd + pgd_index(restore_jump_address), new_pgd);
 	}
 
 	return 0;

_

#6 lore raw

Re: [PATCH 05/11] x86/mm: do not auto-massage page protections

Quoted 33 lines · Dave Hansen

From: Dave Hansen <[email protected]>

A PTE is constructed from a physical address and a pgprotval_t. __PAGE_KERNEL, for instance, is a pgprot_t and must be converted into a pgprotval_t before it can be used to create a PTE. This is done implicitly within functions like pfn_pte() by massage_pgprot().

However, this makes it very challenging to set bits (and keep them set) if your bit is being filtered out by massage_pgprot().

This moves the bit filtering out of pfn_pte() and friends. For users of PAGE_KERNEL*, filtering will be done automatically inside those macros but for users of __PAGE_KERNEL*, they need to do their own filtering now.

Note that we also just move pfn_pte/pmd/pud() over to check_pgprot() instead of massage_pgprot(). This way, we still *look* for unsupported bits and properly warn about them if we find them. This might happen if an unfiltered __PAGE_KERNEL* value was passed in, for instance.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

Quoted 11 lines
 b/arch/x86/include/asm/pgtable.h |   27 ++++++++++++++++++++++-----
 b/arch/x86/kernel/head64.c       |    2 ++
 b/arch/x86/kernel/ldt.c          |    6 +++++-
 b/arch/x86/mm/ident_map.c        |    3 +++
 b/arch/x86/mm/iomap_32.c         |    6 ++++++
 b/arch/x86/mm/ioremap.c          |    3 +++
 b/arch/x86/mm/kasan_init_64.c    |   14 +++++++++++++-
 b/arch/x86/mm/pgtable.c          |    3 +++
 b/arch/x86/power/hibernate_64.c  |   20 +++++++++++++++-----
 9 files changed, 72 insertions(+), 12 deletions(-)

This fails to build for me when I enable KASLR (RANDOMIZE_BASE), with the following error:

arch/x86/boot/compressed/kaslr_64.o: In function `kernel_ident_mapping_init':
kaslr_64.c:(.text+0x245): undefined reference to `__default_kernel_pte_mask' make[2]: *** [arch/x86/boot/compressed/vmlinux] Error 1
make[1]: *** [arch/x86/boot/compressed/vmlinux] Error 2
make: *** [bzImage] Error 2

Thanks,
Tom

Quoted 235 lines

diff -puN arch/x86/include/asm/pgtable.h~x86-no-auto-massage arch/x86/include/asm/pgtable.h

arch/x86/include/asm/pgtable.h renamed from arch/x86/include/asm/pgtable.h~x86-no-auto-massage +22 -5
@@ -526,22 +526,39 @@ static inline pgprotval_t massage_pgprot
 	return protval;
 }
 
+static inline pgprotval_t check_pgprot(pgprot_t pgprot)
+{
+	pgprotval_t massaged_val = massage_pgprot(pgprot);
+
+	/* mmdebug.h can not be included here because of dependencies */
+#ifdef CONFIG_DEBUG_VM
+	WARN_ONCE(pgprot_val(pgprot) != massaged_val,
+		  "attempted to set unsupported pgprot: %016lx "
+		  "bits: %016lx supported: %016lx\n",
+		  pgprot_val(pgprot),
+		  pgprot_val(pgprot) ^ massaged_val,
+		  __supported_pte_mask);
+#endif
+
+	return massaged_val;
+}
+
 static inline pte_t pfn_pte(unsigned long page_nr, pgprot_t pgprot)
 {
 	return __pte(((phys_addr_t)page_nr << PAGE_SHIFT) |
-		     massage_pgprot(pgprot));
+		     check_pgprot(pgprot));
 }
 
 static inline pmd_t pfn_pmd(unsigned long page_nr, pgprot_t pgprot)
 {
 	return __pmd(((phys_addr_t)page_nr << PAGE_SHIFT) |
-		     massage_pgprot(pgprot));
+		     check_pgprot(pgprot));
 }
 
 static inline pud_t pfn_pud(unsigned long page_nr, pgprot_t pgprot)
 {
 	return __pud(((phys_addr_t)page_nr << PAGE_SHIFT) |
-		     massage_pgprot(pgprot));
+		     check_pgprot(pgprot));
 }
 
 static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
@@ -553,7 +570,7 @@ static inline pte_t pte_modify(pte_t pte
 	 * the newprot (if present):
 	 */
 	val &= _PAGE_CHG_MASK;
-	val |= massage_pgprot(newprot) & ~_PAGE_CHG_MASK;
+	val |= check_pgprot(newprot) & ~_PAGE_CHG_MASK;
 
 	return __pte(val);
 }
@@ -563,7 +580,7 @@ static inline pmd_t pmd_modify(pmd_t pmd
 	pmdval_t val = pmd_val(pmd);
 
 	val &= _HPAGE_CHG_MASK;
-	val |= massage_pgprot(newprot) & ~_HPAGE_CHG_MASK;
+	val |= check_pgprot(newprot) & ~_HPAGE_CHG_MASK;
 
 	return __pmd(val);
 }

diff -puN arch/x86/kernel/head64.c~x86-no-auto-massage arch/x86/kernel/head64.c

arch/x86/kernel/head64.c renamed from arch/x86/kernel/head64.c~x86-no-auto-massage +2 -0
@@ -195,6 +195,8 @@ unsigned long __head __startup_64(unsign
 	pud[i + 1] = (pudval_t)pmd + pgtable_flags;
 
 	pmd_entry = __PAGE_KERNEL_LARGE_EXEC & ~_PAGE_GLOBAL;
+	/* Filter out unsupported __PAGE_KERNEL_* bits: */
+	pmd_entry &= __supported_pte_mask;
 	pmd_entry += sme_get_me_mask();
 	pmd_entry +=  physaddr;
 

diff -puN arch/x86/kernel/ldt.c~x86-no-auto-massage arch/x86/kernel/ldt.c

arch/x86/kernel/ldt.c renamed from arch/x86/kernel/ldt.c~x86-no-auto-massage +5 -1
@@ -145,6 +145,7 @@ map_ldt_struct(struct mm_struct *mm, str
 		unsigned long offset = i << PAGE_SHIFT;
 		const void *src = (char *)ldt->entries + offset;
 		unsigned long pfn;
+		pgprot_t pte_prot;
 		pte_t pte, *ptep;
 
 		va = (unsigned long)ldt_slot_va(slot) + offset;
@@ -163,7 +164,10 @@ map_ldt_struct(struct mm_struct *mm, str
 		 * target via some kernel interface which misses a
 		 * permission check.
 		 */
-		pte = pfn_pte(pfn, __pgprot(__PAGE_KERNEL_RO & ~_PAGE_GLOBAL));
+	       	pte_prot = __pgprot(__PAGE_KERNEL_RO & ~_PAGE_GLOBAL);
+		/* Filter out unsuppored __PAGE_KERNEL* bits: */
+		pgprot_val(pte_prot) |= __supported_pte_mask;
+		pte = pfn_pte(pfn, pte_prot);
 		set_pte_at(mm, va, ptep, pte);
 		pte_unmap_unlock(ptep, ptl);
 	}

diff -puN arch/x86/mm/ident_map.c~x86-no-auto-massage arch/x86/mm/ident_map.c

arch/x86/mm/ident_map.c renamed from arch/x86/mm/ident_map.c~x86-no-auto-massage +3 -0
@@ -98,6 +98,9 @@ int kernel_ident_mapping_init(struct x86
 	if (!info->kernpg_flag)
 		info->kernpg_flag = _KERNPG_TABLE;
 
+	/* Filter out unsupported __PAGE_KERNEL_* bits: */
+	info->kernpg_flag &= __default_kernel_pte_mask;
+
 	for (; addr < end; addr = next) {
 		pgd_t *pgd = pgd_page + pgd_index(addr);
 		p4d_t *p4d;

diff -puN arch/x86/mm/iomap_32.c~x86-no-auto-massage arch/x86/mm/iomap_32.c

arch/x86/mm/iomap_32.c renamed from arch/x86/mm/iomap_32.c~x86-no-auto-massage +6 -0
@@ -44,6 +44,9 @@ int iomap_create_wc(resource_size_t base
 		return ret;
 
 	*prot = __pgprot(__PAGE_KERNEL | cachemode2protval(pcm));
+	/* Filter out unsupported __PAGE_KERNEL* bits: */
+	pgprot_val(*prot) &= __default_kernel_pte_mask;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(iomap_create_wc);
@@ -88,6 +91,9 @@ iomap_atomic_prot_pfn(unsigned long pfn,
 		prot = __pgprot(__PAGE_KERNEL |
 				cachemode2protval(_PAGE_CACHE_MODE_UC_MINUS));
 
+	/* Filter out unsupported __PAGE_KERNEL* bits: */
+	pgprot_val(prot) &= __default_kernel_pte_mask;
+
 	return (void __force __iomem *) kmap_atomic_prot_pfn(pfn, prot);
 }
 EXPORT_SYMBOL_GPL(iomap_atomic_prot_pfn);

diff -puN arch/x86/mm/ioremap.c~x86-no-auto-massage arch/x86/mm/ioremap.c

arch/x86/mm/ioremap.c renamed from arch/x86/mm/ioremap.c~x86-no-auto-massage +3 -0
@@ -816,6 +816,9 @@ void __init __early_set_fixmap(enum fixe
 	}
 	pte = early_ioremap_pte(addr);
 
+        /* Sanitize 'prot' against any unsupported bits: */
+        pgprot_val(flags) &= __default_kernel_pte_mask;
+
 	if (pgprot_val(flags))
 		set_pte(pte, pfn_pte(phys >> PAGE_SHIFT, flags));
 	else

diff -puN arch/x86/mm/kasan_init_64.c~x86-no-auto-massage arch/x86/mm/kasan_init_64.c

arch/x86/mm/kasan_init_64.c renamed from arch/x86/mm/kasan_init_64.c~x86-no-auto-massage +13 -1
@@ -269,6 +269,12 @@ void __init kasan_early_init(void)
 	pudval_t pud_val = __pa_nodebug(kasan_zero_pmd) | _KERNPG_TABLE;
 	p4dval_t p4d_val = __pa_nodebug(kasan_zero_pud) | _KERNPG_TABLE;
 
+	/* Mask out unsupported __PAGE_KERNEL bits: */
+	pte_val &= __default_kernel_pte_mask;
+	pmd_val &= __default_kernel_pte_mask;
+	pud_val &= __default_kernel_pte_mask;
+	p4d_val &= __default_kernel_pte_mask;
+
 	for (i = 0; i < PTRS_PER_PTE; i++)
 		kasan_zero_pte[i] = __pte(pte_val);
 
@@ -371,7 +377,13 @@ void __init kasan_init(void)
 	 */
 	memset(kasan_zero_page, 0, PAGE_SIZE);
 	for (i = 0; i < PTRS_PER_PTE; i++) {
-		pte_t pte = __pte(__pa(kasan_zero_page) | __PAGE_KERNEL_RO | _PAGE_ENC);
+		pte_t pte;
+		pgprot_t prot;
+
+		prot = __pgprot(__PAGE_KERNEL_RO | _PAGE_ENC);
+		pgprot_val(prot) &= __default_kernel_pte_mask;
+
+		pte = __pte(__pa(kasan_zero_page) | pgprot_val(prot));
 		set_pte(&kasan_zero_pte[i], pte);
 	}
 	/* Flush TLBs again to be sure that write protection applied. */

diff -puN arch/x86/mm/pgtable.c~x86-no-auto-massage arch/x86/mm/pgtable.c

arch/x86/mm/pgtable.c renamed from arch/x86/mm/pgtable.c~x86-no-auto-massage +3 -0
@@ -583,6 +583,9 @@ void __native_set_fixmap(enum fixed_addr
 void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
 		       pgprot_t flags)
 {
+	/* Sanitize 'prot' against any unsupported bits: */
+	pgprot_val(flags) &= __default_kernel_pte_mask;
+
 	__native_set_fixmap(idx, pfn_pte(phys >> PAGE_SHIFT, flags));
 }
 

diff -puN arch/x86/power/hibernate_64.c~x86-no-auto-massage arch/x86/power/hibernate_64.c

arch/x86/power/hibernate_64.c renamed from arch/x86/power/hibernate_64.c~x86-no-auto-massage +15 -5
@@ -51,6 +51,12 @@ static int set_up_temporary_text_mapping
 	pmd_t *pmd;
 	pud_t *pud;
 	p4d_t *p4d = NULL;
+	pgprot_t pgtable_prot = __pgprot(_KERNPG_TABLE);
+	pgprot_t pmd_text_prot = __pgprot(__PAGE_KERNEL_LARGE_EXEC);
+
+	/* Filter out unsupported __PAGE_KERNEL* bits: */
+	pgprot_val(pmd_text_prot) &= __default_kernel_pte_mask;
+	pgprot_val(pgtable_prot)  &= __default_kernel_pte_mask;
 
 	/*
 	 * The new mapping only has to cover the page containing the image
@@ -81,15 +87,19 @@ static int set_up_temporary_text_mapping
 		return -ENOMEM;
 
 	set_pmd(pmd + pmd_index(restore_jump_address),
-		__pmd((jump_address_phys & PMD_MASK) | __PAGE_KERNEL_LARGE_EXEC));
+		__pmd((jump_address_phys & PMD_MASK) | pgprot_val(pmd_text_prot)));
 	set_pud(pud + pud_index(restore_jump_address),
-		__pud(__pa(pmd) | _KERNPG_TABLE));
+		__pud(__pa(pmd) | pgprot_val(pgtable_prot)));
 	if (p4d) {
-		set_p4d(p4d + p4d_index(restore_jump_address), __p4d(__pa(pud) | _KERNPG_TABLE));
-		set_pgd(pgd + pgd_index(restore_jump_address), __pgd(__pa(p4d) | _KERNPG_TABLE));
+		p4d_t new_p4d = __p4d(__pa(pud) | pgprot_val(pgtable_prot));
+		pgd_t new_pgd = __pgd(__pa(p4d) | pgprot_val(pgtable_prot));
+
+		set_p4d(p4d + p4d_index(restore_jump_address), new_p4d);
+		set_pgd(pgd + pgd_index(restore_jump_address), new_pgd);
 	} else {
 		/* No p4d for 4-level paging: point the pgd to the pud page table */
-		set_pgd(pgd + pgd_index(restore_jump_address), __pgd(__pa(pud) | _KERNPG_TABLE));
+		pgd_t new_pgd = __pgd(__pa(p4d) | pgprot_val(pgtable_prot));
+		set_pgd(pgd + pgd_index(restore_jump_address), new_pgd);
 	}
 
 	return 0;

_

author maintainer
#7 lore raw

PATCH v4 6/11 x86/mm: remove extra filtering in pageattr code

In reply to Dave Hansen

From: Dave Hansen <[email protected]>

The pageattr code has a mode where it can set or clear PTE bits in existing PTEs, so the page protections of the *new* PTEs come from one of two places:
1. The set/clear masks: cpa->mask_clr / cpa->mask_set
2. The existing PTE

We filter ->mask_set/clr for supported PTE bits at entry to __change_page_attr() so we never need to filter them again.

The only other place permissions can come from is an existing PTE and those already presumably have good bits. We do not need to filter them again.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/mm/pageattr.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
arch/x86/mm/pageattr.c renamed from arch/x86/mm/pageattr.c~x86-pageattr-dont-filter-global +2 -4
diff -puN arch/x86/mm/pageattr.c~x86-pageattr-dont-filter-global arch/x86/mm/pageattr.c
@@ -598,7 +598,6 @@ try_preserve_large_page(pte_t *kpte, uns
 	req_prot = pgprot_clear_protnone_bits(req_prot);
 	if (pgprot_val(req_prot) & _PAGE_PRESENT)
 		pgprot_val(req_prot) |= _PAGE_PSE;
-	req_prot = canon_pgprot(req_prot);
 
 	/*
 	 * old_pfn points to the large page base pfn. So we need
@@ -718,7 +717,7 @@ __split_large_page(struct cpa_data *cpa,
 	 */
 	pfn = ref_pfn;
 	for (i = 0; i < PTRS_PER_PTE; i++, pfn += pfninc)
-		set_pte(&pbase[i], pfn_pte(pfn, canon_pgprot(ref_prot)));
+		set_pte(&pbase[i], pfn_pte(pfn, ref_prot));
 
 	if (virt_addr_valid(address)) {
 		unsigned long pfn = PFN_DOWN(__pa(address));
@@ -935,7 +934,6 @@ static void populate_pte(struct cpa_data
 	pte = pte_offset_kernel(pmd, start);
 
 	pgprot = pgprot_clear_protnone_bits(pgprot);
-	pgprot = canon_pgprot(pgprot);
 
 	while (num_pages-- && start < end) {
 		set_pte(pte, pfn_pte(cpa->pfn, pgprot));
@@ -1234,7 +1232,7 @@ repeat:
 		 * after all we're only going to change it's attributes
 		 * not the memory it points to
 		 */
-		new_pte = pfn_pte(pfn, canon_pgprot(new_prot));
+		new_pte = pfn_pte(pfn, new_prot);
 		cpa->pfn = pfn;
 		/*
 		 * Do we really change anything ?

_

author maintainer
#8 lore raw

PATCH v4 7/11 x86/mm: comment _PAGE_GLOBAL mystery

In reply to Dave Hansen

From: Dave Hansen <[email protected]>

I was mystified as to where the _PAGE_GLOBAL in the kernel page tables for kernel text came from. I audited all the places I could find, but I missed one: head_64.S.

The page tables that we create in here live for a long time, and they also have _PAGE_GLOBAL set, despite whether the processor supports it or not. It's harmless, and we got *lucky* that the pageattr code accidentally clears it when we wipe it out of __supported_pte_mask and then later try to mark kernel text read-only.

Comment some of these properties to make it easier to find and understand in the future.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/kernel/head_64.S |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)
arch/x86/kernel/head_64.S renamed from arch/x86/kernel/head_64.S~comment-global-page +10 -1
diff -puN arch/x86/kernel/head_64.S~comment-global-page arch/x86/kernel/head_64.S
@@ -399,8 +399,13 @@ NEXT_PAGE(level3_ident_pgt)
 	.quad	level2_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE_NOENC
 	.fill	511, 8, 0
 NEXT_PAGE(level2_ident_pgt)
-	/* Since I easily can, map the first 1G.
+	/*
+	 * Since I easily can, map the first 1G.
 	 * Don't set NX because code runs from these pages.
+	 *
+	 * Note: This sets _PAGE_GLOBAL despite whether
+	 * the CPU supports it or it is enabled.  But,
+	 * the CPU should ignore the bit.
 	 */
 	PMDS(0, __PAGE_KERNEL_IDENT_LARGE_EXEC, PTRS_PER_PMD)
 #else
@@ -431,6 +436,10 @@ NEXT_PAGE(level2_kernel_pgt)
 	 * (NOTE: at +512MB starts the module area, see MODULES_VADDR.
 	 *  If you want to increase this then increase MODULES_VADDR
 	 *  too.)
+	 *
+	 *  This table is eventually used by the kernel during normal
+	 *  runtime.  Care must be taken to clear out undesired bits
+	 *  later, like _PAGE_RW or _PAGE_GLOBAL in some cases.
 	 */
 	PMDS(0, __PAGE_KERNEL_LARGE_EXEC,
 		KERNEL_IMAGE_SIZE/PMD_SIZE)

_

author maintainer
#9 lore raw

PATCH v4 8/11 x86/mm: do not forbid _PAGE_RW before init for __ro_after_init

In reply to Dave Hansen

From: Dave Hansen <[email protected]>

__ro_after_init data gets stuck in the .rodata section. That's normally fine because the kernel itself manages the R/W properties.

But, if we run __change_page_attr() on an area which is __ro_after_init, the .rodata checks will trigger and force the area to be immediately read-only, even if it is early-ish in boot. This caused problems when trying to clear the _PAGE_GLOBAL bit for these area in the PTI code:
it cleared _PAGE_GLOBAL like I asked, but also took it up on itself to clear _PAGE_RW. The kernel then oopses the next time it wrote to a __ro_after_init data structure.

To fix this, add the kernel_set_to_readonly check, just like we have for kernel text, just a few lines below in this function.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/mm/pageattr.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
arch/x86/mm/pageattr.c renamed from arch/x86/mm/pageattr.c~check-kernel_set_to_readonly +4 -2
diff -puN arch/x86/mm/pageattr.c~check-kernel_set_to_readonly arch/x86/mm/pageattr.c
@@ -298,9 +298,11 @@ static inline pgprot_t static_protection
 
 	/*
 	 * The .rodata section needs to be read-only. Using the pfn
-	 * catches all aliases.
+	 * catches all aliases.  This also includes __ro_after_init,
+	 * so do not enforce until kernel_set_to_readonly is true.
 	 */
-	if (within(pfn, __pa_symbol(__start_rodata) >> PAGE_SHIFT,
+	if (kernel_set_to_readonly &&
+	    within(pfn, __pa_symbol(__start_rodata) >> PAGE_SHIFT,
 		   __pa_symbol(__end_rodata) >> PAGE_SHIFT))
 		pgprot_val(forbidden) |= _PAGE_RW;
 

_

author maintainer
#10 lore raw

PATCH v4 9/11 x86/pti: enable global pages for shared areas

In reply to Dave Hansen

From: Dave Hansen <[email protected]>

The entry/exit text and cpu_entry_area are mapped into userspace and the kernel. But, they are not _PAGE_GLOBAL. This creates unnecessary TLB misses.

Add the _PAGE_GLOBAL flag for these areas.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/mm/cpu_entry_area.c |   10 +++++++++-
 b/arch/x86/mm/pti.c            |   14 +++++++++++++-
 2 files changed, 22 insertions(+), 2 deletions(-)
arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +9 -1
diff -puN arch/x86/mm/cpu_entry_area.c~kpti-why-no-global arch/x86/mm/cpu_entry_area.c
@@ -27,8 +27,16 @@ EXPORT_SYMBOL(get_cpu_entry_area);
 void cea_set_pte(void *cea_vaddr, phys_addr_t pa, pgprot_t flags)
 {
 	unsigned long va = (unsigned long) cea_vaddr;
+	pte_t pte = pfn_pte(pa >> PAGE_SHIFT, flags);
 
-	set_pte_vaddr(va, pfn_pte(pa >> PAGE_SHIFT, flags));
+	/*
+	 * The cpu_entry_area is shared between the user and kernel
+	 * page tables.  All of its ptes can safely be global.
+	 */
+	if (boot_cpu_has(X86_FEATURE_PGE))
+		pte = pte_set_flags(pte, _PAGE_GLOBAL);
+
+	set_pte_vaddr(va, pte);
 }
 
 static void __init
arch/x86/mm/pti.c renamed from arch/x86/mm/pti.c~kpti-why-no-global +13 -1
diff -puN arch/x86/mm/pti.c~kpti-why-no-global arch/x86/mm/pti.c
@@ -300,6 +300,18 @@ pti_clone_pmds(unsigned long start, unsi
 			return;
 
 		/*
+		 * Setting 'target_pmd' below creates a mapping in both
+		 * the user and kernel page tables.  It is effectively
+		 * global, so set it as global in both copies.  Note:
+		 * the X86_FEATURE_PGE check is not _required_ because
+		 * the CPU ignores _PAGE_GLOBAL when PGE is not
+		 * supported.  The check keeps consistentency with
+		 * code that only set this bit when supported.
+		 */
+		if (boot_cpu_has(X86_FEATURE_PGE))
+			*pmd = pmd_set_flags(*pmd, _PAGE_GLOBAL);
+
+		/*
 		 * Copy the PMD.  That is, the kernelmode and usermode
 		 * tables will share the last-level page tables of this
 		 * address range
@@ -348,7 +360,7 @@ static void __init pti_clone_entry_text(
 {
 	pti_clone_pmds((unsigned long) __entry_text_start,
 			(unsigned long) __irqentry_text_end,
-		       _PAGE_RW | _PAGE_GLOBAL);
+		       _PAGE_RW);
 }
 
 /*

_

#11 lore raw

Re: [PATCH 09/11] x86/pti: enable global pages for shared areas

Quoted 30 lines · Dave Hansen

From: Dave Hansen <[email protected]>

The entry/exit text and cpu_entry_area are mapped into userspace and the kernel. But, they are not _PAGE_GLOBAL. This creates unnecessary TLB misses.

Add the _PAGE_GLOBAL flag for these areas.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

b/arch/x86/mm/cpu_entry_area.c |   10 +++++++++-
b/arch/x86/mm/pti.c            |   14 +++++++++++++-
2 files changed, 22 insertions(+), 2 deletions(-)

diff -puN arch/x86/mm/cpu_entry_area.c~kpti-why-no-global arch/x86/mm/cpu_entry_area.c

arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +0 -0
@@ -27,8 +27,16 @@ EXPORT_SYMBOL(get_cpu_entry_area);
 void cea_set_pte(void *cea_vaddr, phys_addr_t pa, pgprot_t flags)
 {

arch/x86/mm/cpu_entry_area.c EXPORT_SYMBOL(get_cpu_entry_area); · line 37

Quoted 10 lines
arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +7 -1
 	unsigned long va = (unsigned long) cea_vaddr;
+	pte_t pte = pfn_pte(pa >> PAGE_SHIFT, flags);
 
-	set_pte_vaddr(va, pfn_pte(pa >> PAGE_SHIFT, flags));
+	/*
+	 * The cpu_entry_area is shared between the user and kernel
+	 * page tables.  All of its ptes can safely be global.
+	 */
+	if (boot_cpu_has(X86_FEATURE_PGE))
+		pte = pte_set_flags(pte, _PAGE_GLOBAL);

I think it would be safer to check that the PTE is indeed present before setting _PAGE_GLOBAL. For example, percpu_setup_debug_store() sets PAGE_NONE for non-present entries. In this case, since PAGE_NONE and PAGE_GLOBAL use the same bit, everything would be fine, but it might cause bugs one day.

Quoted 12 lines
arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +2 -0
+
+	set_pte_vaddr(va, pte);
 }
 

static void __init
diff -puN arch/x86/mm/pti.c~kpti-why-no-global arch/x86/mm/pti.c

arch/x86/mm/pti.c renamed from arch/x86/mm/pti.c~kpti-why-no-global +0 -0
@@ -300,6 +300,18 @@ pti_clone_pmds(unsigned long start, unsi
 			return;
 
 		/*

arch/x86/mm/pti.c pti_clone_pmds(unsigned long start, unsi · line 312

Quoted 10 lines
arch/x86/mm/pti.c renamed from arch/x86/mm/pti.c~kpti-why-no-global +10 -0
+		 * Setting 'target_pmd' below creates a mapping in both
+		 * the user and kernel page tables.  It is effectively
+		 * global, so set it as global in both copies.  Note:
+		 * the X86_FEATURE_PGE check is not _required_ because
+		 * the CPU ignores _PAGE_GLOBAL when PGE is not
+		 * supported.  The check keeps consistentency with
+		 * code that only set this bit when supported.
+		 */
+		if (boot_cpu_has(X86_FEATURE_PGE))
+			*pmd = pmd_set_flags(*pmd, _PAGE_GLOBAL);

Same here.

author maintainer
#12 lore raw

Re: [PATCH 09/11] x86/pti: enable global pages for shared areas

Quoted 35 lines · Nadav Amit
Quoted 33 lines · Dave Hansen

From: Dave Hansen <[email protected]>

The entry/exit text and cpu_entry_area are mapped into userspace and the kernel. But, they are not _PAGE_GLOBAL. This creates unnecessary TLB misses.

Add the _PAGE_GLOBAL flag for these areas.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

b/arch/x86/mm/cpu_entry_area.c |   10 +++++++++-
b/arch/x86/mm/pti.c            |   14 +++++++++++++-
2 files changed, 22 insertions(+), 2 deletions(-)

diff -puN arch/x86/mm/cpu_entry_area.c~kpti-why-no-global arch/x86/mm/cpu_entry_area.c

arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +1 -0
@@ -27,8 +27,16 @@ EXPORT_SYMBOL(get_cpu_entry_area);
 void cea_set_pte(void *cea_vaddr, phys_addr_t pa, pgprot_t flags)
 {
 	unsigned long va = (unsigned long) cea_vaddr;
+	pte_t pte = pfn_pte(pa >> PAGE_SHIFT, flags);
 
Quoted 12 lines

arch/x86/mm/cpu_entry_area.c EXPORT_SYMBOL(get_cpu_entry_area); · line 37

Quoted 7 lines
arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +6 -1
-	set_pte_vaddr(va, pfn_pte(pa >> PAGE_SHIFT, flags));
+	/*
+	 * The cpu_entry_area is shared between the user and kernel
+	 * page tables.  All of its ptes can safely be global.
+	 */
+	if (boot_cpu_has(X86_FEATURE_PGE))
+		pte = pte_set_flags(pte, _PAGE_GLOBAL);

I think it would be safer to check that the PTE is indeed present before setting _PAGE_GLOBAL. For example, percpu_setup_debug_store() sets PAGE_NONE for non-present entries. In this case, since PAGE_NONE and PAGE_GLOBAL use the same bit, everything would be fine, but it might cause bugs one day.

That's a reasonable safety thing to add, I think.

But, looking at it, I am wondering why we did this in percpu_setup_debug_store():

        for (; npages; npages--, cea += PAGE_SIZE)
                cea_set_pte(cea, 0, PAGE_NONE);

Did we really want that to be PAGE_NONE, or was it supposed to create a PTE that returns true for pte_none()?

Quoted 5 lines
Quoted 5 lines
arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +4 -0
 		/*
+		 * Setting 'target_pmd' below creates a mapping in both
+		 * the user and kernel page tables.  It is effectively
+		 * global, so set it as global in both copies.  Note:
+		 * the X86_FEATURE_PGE check is not _required_ because
Quoted 8 lines

arch/x86/mm/cpu_entry_area.c EXPORT_SYMBOL(get_cpu_entry_area);

Quoted 6 lines
arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +6 -0
+		 * the CPU ignores _PAGE_GLOBAL when PGE is not
+		 * supported.  The check keeps consistentency with
+		 * code that only set this bit when supported.
+		 */
+		if (boot_cpu_has(X86_FEATURE_PGE))
+			*pmd = pmd_set_flags(*pmd, _PAGE_GLOBAL);

Same here.

Is there a reason that the pmd_none() check above this does not work?

#13 lore raw

Re: [PATCH 09/11] x86/pti: enable global pages for shared areas

Quoted 50 lines · Dave Hansen
Quoted 34 lines · Nadav Amit
Quoted 32 lines · Dave Hansen

From: Dave Hansen <[email protected]>

The entry/exit text and cpu_entry_area are mapped into userspace and the kernel. But, they are not _PAGE_GLOBAL. This creates unnecessary TLB misses.

Add the _PAGE_GLOBAL flag for these areas.

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

b/arch/x86/mm/cpu_entry_area.c |   10 +++++++++-
b/arch/x86/mm/pti.c            |   14 +++++++++++++-
2 files changed, 22 insertions(+), 2 deletions(-)

diff -puN arch/x86/mm/cpu_entry_area.c~kpti-why-no-global arch/x86/mm/cpu_entry_area.c

arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +1 -0
@@ -27,8 +27,16 @@ EXPORT_SYMBOL(get_cpu_entry_area);
 void cea_set_pte(void *cea_vaddr, phys_addr_t pa, pgprot_t flags)
 {
 	unsigned long va = (unsigned long) cea_vaddr;
+	pte_t pte = pfn_pte(pa >> PAGE_SHIFT, flags);
 
Quoted 12 lines

arch/x86/mm/cpu_entry_area.c EXPORT_SYMBOL(get_cpu_entry_area); · line 37

Quoted 7 lines
arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +6 -1
-	set_pte_vaddr(va, pfn_pte(pa >> PAGE_SHIFT, flags));
+	/*
+	 * The cpu_entry_area is shared between the user and kernel
+	 * page tables.  All of its ptes can safely be global.
+	 */
+	if (boot_cpu_has(X86_FEATURE_PGE))
+		pte = pte_set_flags(pte, _PAGE_GLOBAL);

I think it would be safer to check that the PTE is indeed present before setting _PAGE_GLOBAL. For example, percpu_setup_debug_store() sets PAGE_NONE for non-present entries. In this case, since PAGE_NONE and PAGE_GLOBAL use the same bit, everything would be fine, but it might cause bugs one day.

That's a reasonable safety thing to add, I think.

Quoted 8 lines

But, looking at it, I am wondering why we did this in percpu_setup_debug_store():

       for (; npages; npages--, cea += PAGE_SIZE)
               cea_set_pte(cea, 0, PAGE_NONE);

Did we really want that to be PAGE_NONE, or was it supposed to create a PTE that returns true for pte_none()?

I yield it to others to answer...

Quoted 15 lines
Quoted 14 lines

arch/x86/mm/cpu_entry_area.c EXPORT_SYMBOL(get_cpu_entry_area);

Quoted 12 lines
arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +10 -0
 /*
+		 * Setting 'target_pmd' below creates a mapping in both
+		 * the user and kernel page tables.  It is effectively
+		 * global, so set it as global in both copies.  Note:
+		 * the X86_FEATURE_PGE check is not _required_ because
+		 * the CPU ignores _PAGE_GLOBAL when PGE is not
+		 * supported.  The check keeps consistentency with
+		 * code that only set this bit when supported.
+		 */
+		if (boot_cpu_has(X86_FEATURE_PGE))
+			*pmd = pmd_set_flags(*pmd, _PAGE_GLOBAL);

Same here.

Quoted 1 line

Is there a reason that the pmd_none() check above this does not work?

For any practical reasons, right now, it should be fine. But pmd_none() will not save us if _PAGE_PROTNONE ever changes, for example. Note that the check is with pmd_none() and not pmd_protnone().

#14 lore raw

Re: [PATCH 09/11] x86/pti: enable global pages for shared areas

Quoted 26 lines · Nadav Amit
Quoted 25 lines · Dave Hansen
Quoted 6 lines · Nadav Amit
Quoted 5 lines · Dave Hansen

void cea_set_pte(void *cea_vaddr, phys_addr_t pa, pgprot_t flags)

{
	unsigned long va = (unsigned long) cea_vaddr;
arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +1 -0
+	pte_t pte = pfn_pte(pa >> PAGE_SHIFT, flags);
Quoted 12 lines

arch/x86/mm/cpu_entry_area.c EXPORT_SYMBOL(get_cpu_entry_area); · line 37

Quoted 7 lines
arch/x86/mm/cpu_entry_area.c renamed from arch/x86/mm/cpu_entry_area.c~kpti-why-no-global +6 -1
-	set_pte_vaddr(va, pfn_pte(pa >> PAGE_SHIFT, flags));
+	/*
+	 * The cpu_entry_area is shared between the user and kernel
+	 * page tables.  All of its ptes can safely be global.
+	 */
+	if (boot_cpu_has(X86_FEATURE_PGE))
+		pte = pte_set_flags(pte, _PAGE_GLOBAL);

I think it would be safer to check that the PTE is indeed present before setting _PAGE_GLOBAL. For example, percpu_setup_debug_store() sets PAGE_NONE for non-present entries. In this case, since PAGE_NONE and PAGE_GLOBAL use the same bit, everything would be fine, but it might cause bugs one day.

That's a reasonable safety thing to add, I think.

But, looking at it, I am wondering why we did this in percpu_setup_debug_store():

Quoted 7 lines
Quoted 5 lines
       for (; npages; npages--, cea += PAGE_SIZE)
               cea_set_pte(cea, 0, PAGE_NONE);

Did we really want that to be PAGE_NONE, or was it supposed to create a PTE that returns true for pte_none()?

I yield it to others to answer...

My bad. I should have used pgprot(0).

Thanks,

tglx

author maintainer
#15 lore raw

PATCH v4 10/11 x86/pti: never implicitly clear _PAGE_GLOBAL for kernel image

In reply to Dave Hansen

From: Dave Hansen <[email protected]>

Summary:

In current kernels, with PTI enabled, no pages are marked Global. This potentially increases TLB misses. But, the mechanism by which the Global bit is set and cleared is rather haphazard. This patch makes the process more explicit. In the end, it leaves us with Global entries in the page tables for the areas truly shared by userspace and kernel and increases TLB hit rates.

The place this patch really shines in on systems without PCIDs. In this case, we are using an lseek microbenchmark[1] to see how a reasonably non-trivial syscall behaves. Higher is better:

No Global pages (baseline): 6077741 lseeks/sec 88 Global Pages (this set): 7528609 lseeks/sec (+23.9%)

On a modern Skylake desktop with PCIDs, the benefits are tangible, but not huge for a kernel compile (lower is better):

No Global pages (baseline): 186.951 seconds time elapsed  ( +-  0.35% )
28 Global pages (this set): 185.756 seconds time elapsed  ( +-  0.09% )
                             -1.195 seconds (-0.64%)

I also re-checked everything using the lseek1 test[1]:

No Global pages (baseline): 15783951 lseeks/sec
28 Global pages (this set): 16054688 lseeks/sec

			     +270737 lseeks/sec (+1.71%)

The effect is more visible, but still modest.

Details:

The kernel page tables are inherited from head_64.S which rudely marks them as _PAGE_GLOBAL. For PTI, we have been relying on the grace of $DEITY and some insane behavior in pageattr.c to clear _PAGE_GLOBAL. This patch tries to do better.

First, stop filtering out "unsupported" bits from being cleared in the pageattr code. It's fine to filter out *setting* these bits but it is insane to keep us from clearing them.

Then, *explicitly* go clear _PAGE_GLOBAL from the kernel identity map. Do not rely on pageattr to do it magically.

After this patch, we can see that "GLB" shows up in each copy of the page tables, that we have the same number of global entries in each and that they are the *same* entries.

# grep -c GLB /sys/kernel/debug/page_tables/*
/sys/kernel/debug/page_tables/current_kernel:11
/sys/kernel/debug/page_tables/current_user:11
/sys/kernel/debug/page_tables/kernel:11
# for f in `ls /sys/kernel/debug/page_tables/`; do grep GLB /sys/kernel/debug/page_tables/$f > $f.GLB; done
# md5sum *.GLB
9caae8ad6a1fb53aca2407ec037f612d  current_kernel.GLB
9caae8ad6a1fb53aca2407ec037f612d  current_user.GLB
9caae8ad6a1fb53aca2407ec037f612d  kernel.GLB

A quick visual audit also shows that all the entries make sense. 0xfffffe0000000000 is the cpu_entry_area and 0xffffffff81c00000 is the entry/exit text:

# grep -c GLB /sys/kernel/debug/page_tables/current_user
0xfffffe0000000000-0xfffffe0000002000           8K     ro                 GLB NX pte
0xfffffe0000002000-0xfffffe0000003000           4K     RW                 GLB NX pte
0xfffffe0000003000-0xfffffe0000006000          12K     ro                 GLB NX pte
0xfffffe0000006000-0xfffffe0000007000           4K     ro                 GLB x  pte
0xfffffe0000007000-0xfffffe000000d000          24K     RW                 GLB NX pte
0xfffffe000002d000-0xfffffe000002e000           4K     ro                 GLB NX pte
0xfffffe000002e000-0xfffffe000002f000           4K     RW                 GLB NX pte
0xfffffe000002f000-0xfffffe0000032000          12K     ro                 GLB NX pte
0xfffffe0000032000-0xfffffe0000033000           4K     ro                 GLB x  pte
0xfffffe0000033000-0xfffffe0000039000          24K     RW                 GLB NX pte
0xffffffff81c00000-0xffffffff81e00000           2M     ro         PSE     GLB x  pmd

1. https://github.com/antonblanchard/will-it-scale/blob/master/tests/lseek1.c

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/mm/init.c     |    8 +-------
 b/arch/x86/mm/pageattr.c |   12 +++++++++---
 b/arch/x86/mm/pti.c      |   25 +++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 10 deletions(-)
arch/x86/mm/init.c renamed from arch/x86/mm/init.c~clear-global-for-pti +1 -7
diff -puN arch/x86/mm/init.c~clear-global-for-pti arch/x86/mm/init.c
@@ -161,12 +161,6 @@ struct map_range {
 
 static int page_size_mask;
 
-static void enable_global_pages(void)
-{
-	if (!static_cpu_has(X86_FEATURE_PTI))
-		__supported_pte_mask |= _PAGE_GLOBAL;
-}
-
 static void __init probe_page_size_mask(void)
 {
 	/*
@@ -187,7 +181,7 @@ static void __init probe_page_size_mask(
 	__supported_pte_mask &= ~_PAGE_GLOBAL;
 	if (boot_cpu_has(X86_FEATURE_PGE)) {
 		cr4_set_bits_and_update_boot(X86_CR4_PGE);
-		enable_global_pages();
+		__supported_pte_mask |= _PAGE_GLOBAL;
 	}
 
 	/* By the default is everything supported: */
arch/x86/mm/pageattr.c renamed from arch/x86/mm/pageattr.c~clear-global-for-pti +9 -3
diff -puN arch/x86/mm/pageattr.c~clear-global-for-pti arch/x86/mm/pageattr.c
@@ -1411,11 +1411,11 @@ static int change_page_attr_set_clr(unsi
 	memset(&cpa, 0, sizeof(cpa));
 
 	/*
-	 * Check, if we are requested to change a not supported
-	 * feature:
+	 * Check, if we are requested to set a not supported
+	 * feature.  Clearing non-supported features is OK.
 	 */
 	mask_set = canon_pgprot(mask_set);
-	mask_clr = canon_pgprot(mask_clr);
+
 	if (!pgprot_val(mask_set) && !pgprot_val(mask_clr) && !force_split)
 		return 0;
 
@@ -1758,6 +1758,12 @@ int set_memory_4k(unsigned long addr, in
 					__pgprot(0), 1, 0, NULL);
 }
 
+int set_memory_nonglobal(unsigned long addr, int numpages)
+{
+	return change_page_attr_clear(&addr, numpages,
+				      __pgprot(_PAGE_GLOBAL), 0);
+}
+
 static int __set_memory_enc_dec(unsigned long addr, int numpages, bool enc)
 {
 	struct cpa_data cpa;
arch/x86/mm/pti.c renamed from arch/x86/mm/pti.c~clear-global-for-pti +25 -0
diff -puN arch/x86/mm/pti.c~clear-global-for-pti arch/x86/mm/pti.c
@@ -364,6 +364,27 @@ static void __init pti_clone_entry_text(
 }
 
 /*
+ * This is the only user for it and it is not arch-generic like
+ * the other set_memory.h functions.  Just extern it.
+ */
+extern int set_memory_nonglobal(unsigned long addr, int numpages);
+void pti_set_kernel_image_nonglobal(void)
+{
+	/*
+	 * The identity map is created with PMDs, regardless of the
+	 * actual length of the kernel.  We need to clear
+	 * _PAGE_GLOBAL up to a PMD boundary, not just to the end
+	 * of the image.
+	 */
+	unsigned long start = PFN_ALIGN(_text);
+	unsigned long end = ALIGN((unsigned long)_end, PMD_PAGE_SIZE);
+
+	pr_debug("set kernel image non-global\n");
+
+	set_memory_nonglobal(start, (end - start) >> PAGE_SHIFT);
+}
+
+/*
  * Initialize kernel page table isolation
  */
 void __init pti_init(void)
@@ -374,6 +395,10 @@ void __init pti_init(void)
 	pr_info("enabled\n");
 
 	pti_clone_user_shared();
+
+	/* Undo all global bits from the init pagetables in head_64.S: */
+	pti_set_kernel_image_nonglobal();
+	/* Replace some of the global bits just for shared entry text: */
 	pti_clone_entry_text();
 	pti_setup_espfix64();
 	pti_setup_vsyscall();

_

author maintainer
#16 lore raw

PATCH v4 11/11 x86/pti: leave kernel text global for !PCID

In reply to Dave Hansen

Note: This has changed since the last version. It now clones the kernel text PMDs at a much later point and also disables this functionality on AMD K8 processors. Details in the patch.

--

I'm sticking this at the end of the series because it's a bit weird. It can be dropped and the rest of the series is still useful without it.

Global pages are bad for hardening because they potentially let an exploit read the kernel image via a Meltdown-style attack which makes it easier to find gadgets.

But, global pages are good for performance because they reduce TLB misses when making user/kernel transitions, especially when PCIDs are not available, such as on older hardware, or where a hypervisor has disabled them for some reason.

This patch implements a basic, sane policy: If you have PCIDs, you only map a minimal amount of kernel text global. If you do not have PCIDs, you map all kernel text global.

This policy effectively makes PCIDs something that not only adds performance but a little bit of hardening as well.

I ran a simple "lseek" microbenchmark[1] to test the benefit on a modern Atom microserver. Most of the benefit comes from applying the series before this patch ("entry only"), but there is still a signifiant benefit from this patch.

No Global Lines (baseline ): 6077741 lseeks/sec 88 Global Lines (entry only): 7528609 lseeks/sec (+23.9%) 94 Global Lines (this patch): 8433111 lseeks/sec (+38.8%)

1. https://github.com/antonblanchard/will-it-scale/blob/master/tests/lseek1.c

Cc Andrea Arcangeli <[email protected]>
Cc Andy Lutomirski <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Kees Cook <[email protected]>
Cc Hugh Dickins <[email protected]>
Cc Juergen Gross <[email protected]>
Cc Nadav Amit <[email protected]>

---

 b/arch/x86/include/asm/pti.h |    2 +
 b/arch/x86/mm/init_64.c      |    6 +++
 b/arch/x86/mm/pti.c          |   79 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 83 insertions(+), 4 deletions(-)
arch/x86/include/asm/pti.h renamed from arch/x86/include/asm/pti.h~kpti-global-text-option +2 -0
diff -puN arch/x86/include/asm/pti.h~kpti-global-text-option arch/x86/include/asm/pti.h
@@ -6,8 +6,10 @@
 #ifdef CONFIG_PAGE_TABLE_ISOLATION
 extern void pti_init(void);
 extern void pti_check_boottime_disable(void);
+extern void pti_clone_kernel_text(void);
 #else
 static inline void pti_check_boottime_disable(void) { }
+static inline void pti_clone_kernel_text(void) { }
 #endif
 
 #endif /* __ASSEMBLY__ */
arch/x86/mm/init_64.c renamed from arch/x86/mm/init_64.c~kpti-global-text-option +6 -0
diff -puN arch/x86/mm/init_64.c~kpti-global-text-option arch/x86/mm/init_64.c
@@ -1294,6 +1294,12 @@ void mark_rodata_ro(void)
 			(unsigned long) __va(__pa_symbol(_sdata)));
 
 	debug_checkwx();
+
+	/*
+	 * Do this after all of the manipulation of the
+	 * kernel text page tables are complete.
+	 */
+	pti_clone_kernel_text();
 }
 
 int kern_addr_valid(unsigned long addr)
arch/x86/mm/pti.c renamed from arch/x86/mm/pti.c~kpti-global-text-option +75 -4

142 lines not on this page · raw message

_

#17 lore raw

Re: [PATCH 11/11] x86/pti: leave kernel text global for !PCID

Thank you for the patch! Perhaps something to improve:

Hi Dave,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/auto-latest]
[also build test WARNING on next-20180403]
[cannot apply to tip/x86/core v4.16]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Dave-Hansen/Use-global-pages-with-PTI/20180404-135611
reproduce:

        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__

sparse warnings: (new ones prefixed by >>)

>> arch/x86/mm/pti.c:286:1: sparse: symbol 'pti_clone_pmds' was not declared. Should it be static?

arch/x86/mm/pti.c:439:6: sparse: symbol 'pti_set_kernel_image_nonglobal' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

#18 lore raw

[RFC PATCH] x86/pti: pti_clone_pmds can be static

In reply to Dave Hansen
Fixes: a7e2701bf2b2 ("x86/pti: leave kernel text global for !PCID")
Fixes a7e2701bf2b2 ("x86/pti: leave kernel text global for !PCID")
---
 pti.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/mm/pti.c +1 -1
index 3ee9ceb..057c8ff 100644
@@ -282,7 +282,7 @@ static void __init pti_setup_vsyscall(void)
 static void __init pti_setup_vsyscall(void) { }
 #endif
 
-void
+static void
 pti_clone_pmds(unsigned long start, unsigned long end, pmdval_t clear)
 {
 	unsigned long addr;
#19 lore raw
In reply to Dave Hansen
Quoted 5 lines · Dave Hansen

Changes from v3:
* Fix whitespace issue noticed by willy
* Clarify comments about X86_FEATURE_PGE checks
* Clarify commit message around the necessity of _PAGE_GLOBAL filtering when CR4.PGE=0 or PGE is unsupported.

I couldn't see anything odd in this, but I only read the explanations and the patches, and the devil is in the details.

But it all looks sane to me, and the added comments all seemed like good ideas. Plus the performance numbers certainly speak for themselves, even if the big changes are from that Atom microserver that I probably personally wouldn't want to use anyway ;).

So Ack from me, maybe a weak review.

Linus