x86/mm/pti: Robustness updates

From: Song Liu <[email protected]>

pti_clone_pmds() assumes that the supplied address is either:

- properly PUD/PMD aligned
or
- the address is actually mapped which means that independent of the mapping level (PUD/PMD/PTE) the next higher mapping exist.

If that's not the case the unaligned address can be incremented by PUD or PMD size wrongly. All callers supply mapped and/or aligned addresses, but for robustness sake, it's better to handle that case proper and to emit a warning.

Quoted 10 lines · Thomas Gleixner

From: Song Liu <[email protected]>

pti_clone_pmds() assumes that the supplied address is either:

- properly PUD/PMD aligned
or
- the address is actually mapped which means that independent of the mapping level (PUD/PMD/PTE) the next higher mapping exist.

Quoted 4 lines

If that's not the case the unaligned address can be incremented by PUD or PMD size wrongly. All callers supply mapped and/or aligned addresses, but for robustness sake, it's better to handle that case proper and to emit a warning.

Reviewed-by Dave Hansen <[email protected]>

Song, did you ever root-cause the performance regression? I thought there were still some mysteries there.

Quoted 16 lines · Dave Hansen

Quoted 14 lines · Thomas Gleixner

From: Song Liu <[email protected]>

pti_clone_pmds() assumes that the supplied address is either:

- properly PUD/PMD aligned
or
- the address is actually mapped which means that independent of the mapping level (PUD/PMD/PTE) the next higher mapping exist.

If that's not the case the unaligned address can be incremented by PUD or PMD size wrongly. All callers supply mapped and/or aligned addresses, but for robustness sake, it's better to handle that case proper and to emit a warning.

Quoted 4 lines

Reviewed-by Dave Hansen <[email protected]>

Song, did you ever root-cause the performance regression? I thought there were still some mysteries there.

See Peter's series to rework the ftrace code patching ...

Quoted 19 lines · Thomas Gleixner

Quoted 16 lines · Dave Hansen

Quoted 14 lines · Thomas Gleixner

From: Song Liu <[email protected]>

pti_clone_pmds() assumes that the supplied address is either:

- properly PUD/PMD aligned
or
- the address is actually mapped which means that independent of the mapping level (PUD/PMD/PTE) the next higher mapping exist.

If that's not the case the unaligned address can be incremented by PUD or PMD size wrongly. All callers supply mapped and/or aligned addresses, but for robustness sake, it's better to handle that case proper and to emit a warning.

Quoted 6 lines

Quoted 4 lines

Reviewed-by Dave Hansen <[email protected]>

Song, did you ever root-cause the performance regression? I thought there were still some mysteries there.

See Peter's series to rework the ftrace code patching ...

Thanks Thomas.

Yes, in summary, enabling ftrace or kprobe-on-ftrace causes the kernel to split PMDs in kernel text mapping.

Related question: while Peter's patches fix it for 5.3 kernel, they don't apply cleanly over 5.2 kernel (which we are using). So I wonder what is the best solution for 5.2 kernel. May patch also fixes the issue:

https://lore.kernel.org/lkml/[email protected]/

How about we apply this patch to upstream 5.2 kernel?

Quoted 35 lines · Song Liu

Quoted 19 lines · Thomas Gleixner

Quoted 16 lines · Dave Hansen

Quoted 14 lines · Thomas Gleixner

From: Song Liu <[email protected]>

pti_clone_pmds() assumes that the supplied address is either:

- properly PUD/PMD aligned
or
- the address is actually mapped which means that independent of the mapping level (PUD/PMD/PTE) the next higher mapping exist.

If that's not the case the unaligned address can be incremented by PUD or PMD size wrongly. All callers supply mapped and/or aligned addresses, but for robustness sake, it's better to handle that case proper and to emit a warning.

Quoted 6 lines

Quoted 4 lines

Reviewed-by Dave Hansen <[email protected]>

Song, did you ever root-cause the performance regression? I thought there were still some mysteries there.

See Peter's series to rework the ftrace code patching ...

Thanks Thomas.

Yes, in summary, enabling ftrace or kprobe-on-ftrace causes the kernel to split PMDs in kernel text mapping.

Related question: while Peter's patches fix it for 5.3 kernel, they don't apply cleanly over 5.2 kernel (which we are using). So I wonder what is the best solution for 5.2 kernel. May patch also fixes the issue:

Quoted 3 lines

https://lore.kernel.org/lkml/[email protected]/

How about we apply this patch to upstream 5.2 kernel?

That's not how it works. We fix stuff upstream and it gets backported to all affected kernels not just to the one you care about.

Aside of that I really disagree with that hack. You completely fail to explain why that commit in question broke it and instead of fixing the underlying issue you create a horrible workaround.

It took me ~10 minutes to analyze the root cause and I'm just booting the test box with a proper fix which can be actually tagged for stable and can be removed from upstream again once ftrace got moved over to text poke.

I'll post it once it's confirmed to work and I wrote a comprehensible changelog.

ftrace does not use text_poke() for enabling trace functionality. It uses its own mechanism and flips the whole kernel text to RW and back to RO.

The CPA rework removed a loop based check of 4k pages which tried to preserve a large page by checking each 4k page whether the change would actually cover all pages in the large page.

This resulted in endless loops for nothing as in testing it turned out that it actually never preserved anything. Of course testing missed to include ftrace, which is the one and only case which benefitted from the 4k loop.

As a consequence enabling function tracing or ftrace based kprobes results in a full 4k split of the kernel text, which affects iTLB performance.

The kernel RO protection is the only valid case where this can actually preserve large pages.

All other static protections (RO data, data NX, PCI, BIOS) are truly static. So a conflict with those protections which results in a split should only ever happen when a change of memory next to a protected region is attempted. But these conflicts are rightfully splitting the large page to preserve the protected regions. In fact a change to the protected regions itself is a bug and is warned about.

Add an exception for the static protection check for kernel text RO when the to be changed region spawns a full large page which allows to preserve the large mappings. This also prevents the syslog to be spammed about CPA violations when ftrace is used.

The exception needs to be removed once ftrace switched over to text_poke() which avoids the whole issue.

Fixes 585948f4f695 ("x86/mm/cpa: Avoid the 4k pages check completely")

Reported-by Song Liu <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Cc [email protected]

---
arch/x86/mm/pageattr.c | 26 ++++++++++++++++++--------

1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/mm/pageattr.c +18 -8

@@ -516,7 +516,7 @@ static inline void check_conflict(int wa
 */
 static inline pgprot_t static_protections(pgprot_t prot, unsigned long start,
 					  unsigned long pfn, unsigned long npg,
-					  int warnlvl)
+					  unsigned long lpsize, int warnlvl)
 {
 	pgprotval_t forbidden, res;
 	unsigned long end;

@@ -535,9 +535,17 @@ static inline pgprot_t static_protection
 	check_conflict(warnlvl, prot, res, start, end, pfn, "Text NX");
 	forbidden = res;
 
-	res = protect_kernel_text_ro(start, end);
-	check_conflict(warnlvl, prot, res, start, end, pfn, "Text RO");
-	forbidden |= res;
+	/*
+	 * Special case to preserve a large page. If the change spawns the
+	 * full large page mapping then there is no point to split it
+	 * up. Happens with ftrace and is going to be removed once ftrace
+	 * switched to text_poke().
+	 */
+	if (lpsize != (npg * PAGE_SIZE) || (start & (lpsize - 1))) {
+		res = protect_kernel_text_ro(start, end);
+		check_conflict(warnlvl, prot, res, start, end, pfn, "Text RO");
+		forbidden |= res;
+	}
 
 	/* Check the PFN directly */
 	res = protect_pci_bios(pfn, pfn + npg - 1);

@@ -819,7 +827,7 @@ static int __should_split_large_page(pte
 	 * extra conditional required here.
 	 */
 	chk_prot = static_protections(old_prot, lpaddr, old_pfn, numpages,
-				      CPA_CONFLICT);
+				      psize, CPA_CONFLICT);
 
 	if (WARN_ON_ONCE(pgprot_val(chk_prot) != pgprot_val(old_prot))) {
 		/*

@@ -855,7 +863,7 @@ static int __should_split_large_page(pte
 	 * protection requirement in the large page.
 	 */
 	new_prot = static_protections(req_prot, lpaddr, old_pfn, numpages,
-				      CPA_DETECT);
+				      psize, CPA_DETECT);
 
 	/*
 	 * If there is a conflict, split the large page.

@@ -906,7 +914,8 @@ static void split_set_pte(struct cpa_dat
 	if (!cpa->force_static_prot)
 		goto set;
 
-	prot = static_protections(ref_prot, address, pfn, npg, CPA_PROTECT);
+	/* Hand in lpsize = 0 to enforce the protection mechanism */
+	prot = static_protections(ref_prot, address, pfn, npg, 0, CPA_PROTECT);
 
 	if (pgprot_val(prot) == pgprot_val(ref_prot))
 		goto set;

@@ -1503,7 +1512,8 @@ static int __change_page_attr(struct cpa
 		pgprot_val(new_prot) |= pgprot_val(cpa->mask_set);
 
 		cpa_inc_4k_install();
-		new_prot = static_protections(new_prot, address, pfn, 1,
+		/* Hand in lpsize = 0 to enforce the protection mechanism */
+		new_prot = static_protections(new_prot, address, pfn, 1, 0,
 					      CPA_PROTECT);
 
 		new_prot = pgprot_clear_protnone_bits(new_prot);

 arch/x86/mm/pageattr.c |   26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

arch/x86/mm/pageattr.c +2 -4

@@ -516,7 +516,7 @@ static inline void check_conflict(int wa
  */
 static inline pgprot_t static_protections(pgprot_t prot, unsigned long start,
 					  unsigned long pfn, unsigned long npg,
-					  int warnlvl)
+					  unsigned long lpsize, int warnlvl)
 {
 	pgprotval_t forbidden, res;
 	unsigned long end;

@@ -535,9 +535,17 @@ static inline pgprot_t static_protection
 	check_conflict(warnlvl, prot, res, start, end, pfn, "Text NX");
 	forbidden = res;
 
-	res = protect_kernel_text_ro(start, end);
-	check_conflict(warnlvl, prot, res, start, end, pfn, "Text RO");
-	forbidden |= res;
+	/*

arch/x86/mm/pageattr.c +1 -1

 	/* Check the PFN directly */
 	res = protect_pci_bios(pfn, pfn + npg - 1);

@@ -819,7 +827,7 @@ static int __should_split_large_page(pte
 	 * extra conditional required here.
 	 */
 	chk_prot = static_protections(old_prot, lpaddr, old_pfn, numpages,
-				      CPA_CONFLICT);
+				      psize, CPA_CONFLICT);
 
 	if (WARN_ON_ONCE(pgprot_val(chk_prot) != pgprot_val(old_prot))) {
 		/*

From: Song Liu <[email protected]>

pti_clone_pmds() assumes that the supplied address is either:

- properly PUD/PMD aligned
or
- the address is actually mapped which means that independent of the mapping level (PUD/PMD/PTE) the next higher mapping exist.

If that's not the case the unaligned address can be incremented by PUD or PMD size wrongly. All callers supply mapped and/or aligned addresses, but for robustness sake, it's better to handle that case proper and to emit a warning.

arch/x86/mm/pti.c pti_clone_pgtable(unsigned long start, u · line 341

Quoted 5 lines

arch/x86/mm/pti.c +2 -1

 		pmd = pmd_offset(pud, addr);
 		if (pmd_none(*pmd)) {
-			addr += PMD_SIZE;
+			WARN_ON_ONCE(addr & PMD_MASK);
+			addr = round_up(addr + 1, PMD_SIZE);

So given that PUD_MASK and PMD_MASK are masking out the *offset*:

arch/x86/include/asm/pgtable_64_types.h:#define PMD_MASK (~(PMD_SIZE - 1))

Didn't we want something like:

			WARN_ON_ONCE(addr & ~PUD_MASK);

			WARN_ON_ONCE(addr & ~PMD_MASK);

to warn about an unaligned 'addr', or am I misreading the intent here?

From: Song Liu <[email protected]>

pti_clone_pmds() assumes that the supplied address is either:

- properly PUD/PMD aligned
or
- the address is actually mapped which means that independent

From: Song Liu <[email protected]>

pti_clone_pmds() assumes that the supplied address is either:

- properly PUD/PMD aligned
or
- the address is actually mapped which means that independently of the mapping level (PUD/PMD/PTE) the next higher mapping exists.

If that's not the case the unaligned address can be incremented by PUD or PMD size incorrectly. All callers supply mapped and/or aligned addresses, but for the sake of robustness it's better to handle that case properly and to emit a warning.

[ tglx: Rewrote changelog and added WARN_ON_ONCE() ]

Signed-off-by Song Liu <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
V2: Negate P[UM]D_MASK for checking whether the offset part is 0
V3: Fix changelog

---
 arch/x86/mm/pti.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

arch/x86/mm/pti.c +2 -1

@@ -330,13 +330,15 @@ pti_clone_pgtable(unsigned long start, u
 
 		pud = pud_offset(p4d, addr);
 		if (pud_none(*pud)) {
-			addr += PUD_SIZE;
+			WARN_ON_ONCE(addr & ~PUD_MASK);
+			addr = round_up(addr + 1, PUD_SIZE);

arch/x86/mm/pti.c +2 -1

@@ -330,13 +330,15 @@ pti_clone_pgtable(unsigned long start, u
 
 		pud = pud_offset(p4d, addr);
 		if (pud_none(*pud)) {
-			addr += PUD_SIZE;
+			WARN_ON_ONCE(addr & ~PUD_MASK);
+			addr = round_up(addr + 1, PUD_SIZE);
 			continue;

Neither function should be called when PTI is runtime disabled. Make the invocation conditional.

When PTI is disabled at boot time either because the CPU is not affected or PTI has been disabled on the command line, the boot code still calls into pti_finalize() which then unconditionally invokes:

pti_clone_entry_text()
pti_clone_kernel_text()

pti_clone_kernel_text() was called unconditionally before the 32bit support was added and 32bit added the call to pti_clone_entry_text().

The call has no side effects as cloning the page tables into the available second one, which was allocated for PTI does not create damage. But it does not make sense either and in case that this functionality would be extended later this might actually lead to hard to diagnose issue.

Neither function should be called when PTI is runtime disabled. Make the invocation conditional.

Signed-off-by Thomas Gleixner <[email protected]>

When PTI is disabled at boot time either because the CPU is not affected or PTI has been disabled on the command line, the boot code still calls into pti_finalize() which then unconditionally invokes:

pti_clone_entry_text()
pti_clone_kernel_text()

pti_clone_kernel_text() was called unconditionally before the 32bit support was added and 32bit added the call to pti_clone_entry_text().

The call has no side effects as cloning the page tables into the available second one, which was allocated for PTI does not create damage. But it does not make sense either and in case that this functionality would be extended later this might actually lead to hard to diagnose issue.

Neither function should be called when PTI is runtime disabled. Make the invocation conditional.

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/mm/pti.c |    2 ++
 1 file changed, 2 insertions(+)

Following up on the discussions around the patch Song submitted to 'cure' a iTLB related performance regression, I picked up Song's patch which makes clone_page_tables() more robust by handling unaligned addresses proper and added one which prevents calling into the PTI code when PTI is enabled compile time, but disabled at runtime (command line or CPU not affected).

There is no point in calling into those PTI functions unconditionally. The resulting page tables are the same before and after the change which makes sense as the code clones the kernel page table into the secondary page table space which is available but not used when PTI is boot time disabled.

But even if it does not do damage today, this could have nasty side effect when the PTI code is changed, extended etc. later.