Skip to contentSkip to conversation

KPTI: get rid of cpu_entry_area mapping duplication

PATCH In review Andy Lutomirski 3 participants 8 messages Dec 2, 2017

Subsystems (2)
x86 x86 MM
lore
Share & tools
lore

Andy Lutomirski

author maintainer
#0 lore raw

cover KPTI: get rid of cpu_entry_area mapping duplication

I like this variant much better. It might also fix the nasty bug tglx and peterz were chasing.

Andy Lutomirski (2):
Undo the split of setup_cpu_entry_area
x86/kpti: Reference all cpu_entry_area pagetables in the usermode
tables

 arch/x86/include/asm/fixmap.h | 14 +++++---
 arch/x86/include/asm/kpti.h   |  8 +++--
 arch/x86/kernel/cpu/common.c  |  3 --
 arch/x86/kernel/smpboot.c     |  2 ++
 arch/x86/kernel/traps.c       |  6 +++-
 arch/x86/mm/kpti.c            | 82 ++++++++++++++++++++++++++-----------------
 6 files changed, 71 insertions(+), 44 deletions(-)
Signature · 1 line
2.13.6
author maintainer
#1 lore raw

PATCH 1/2 Undo the split of setup_cpu_entry_area

This is obviously a hack. Either the patch should be adjusted back to the version I sent or trap_init should forcibly initialize all PMDs by something like __set_fixmap(..., __mkpte(0)); or however it's spelled.

---
 arch/x86/kernel/smpboot.c | 2 ++
 arch/x86/kernel/traps.c   | 6 +++++-
 2 files changed, 7 insertions(+), 1 deletion(-)
arch/x86/kernel/smpboot.c +2 -0
index 26317716559d..1325844b930a 100644
@@ -1232,8 +1232,10 @@ void __init native_smp_prepare_cpus(unsigned int max_cpus)
 		 * The boot CPU area has been set up in trap_init()
 		 * already.
 		 */
+		/*
 		if (i)
 			setup_cpu_entry_area(i);
+		*/
 	}
 
 	/*
arch/x86/kernel/traps.c +5 -1
index ff4e6b595ae4..0ad92f35a75b 100644
@@ -945,8 +945,12 @@ dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code)
 
 void __init trap_init(void)
 {
+	int cpu;
 	/* Init cpu_entry_area before IST entries are set up */
-	setup_cpu_entry_area(smp_processor_id());
+	for_each_possible_cpu(cpu) {
+		pr_err("setup_cpu_entry_area(%d)\n", cpu);
+		setup_cpu_entry_area(cpu);
+	}
 
 	idt_setup_traps();
Signature · 1 line
2.13.6
#2 lore raw

Re: [PATCH 1/2] Undo the split of setup_cpu_entry_area

Quoted 3 lines · Andy Lutomirski

This is obviously a hack. Either the patch should be adjusted back to the version I sent or trap_init should forcibly initialize all PMDs by something like __set_fixmap(..., __mkpte(0)); or however it's spelled.

I split it because the whole thing crashed when I kept the loop you had because it tried to allocate stuff. Had no time to figure out why, so I went the lazy way of making it "work".

Simplifying the whole mess was on my list anyway.

Thanks,

tglx

#3 lore raw

Re: [PATCH 1/2] Undo the split of setup_cpu_entry_area

Quoted 9 lines · Thomas Gleixner
Quoted 3 lines · Andy Lutomirski

This is obviously a hack. Either the patch should be adjusted back to the version I sent or trap_init should forcibly initialize all PMDs by something like __set_fixmap(..., __mkpte(0)); or however it's spelled.

I split it because the whole thing crashed when I kept the loop you had because it tried to allocate stuff. Had no time to figure out why, so I went the lazy way of making it "work".

[    0.000000]  dump_stack+0x85/0xc5
[    0.000000]  warn_alloc+0x114/0x1c0
[    0.000000]  __alloc_pages_slowpath+0x1089/0x10d0
[    0.000000]  __alloc_pages_nodemask+0x2e8/0x370
[    0.000000]  __get_free_pages+0x10/0x40
[    0.000000]  kpti_shadow_pagetable_walk+0x2b2/0x3e0
[    0.000000]  kpti_add_user_map+0xfe/0x330
[    0.000000]  kpti_add_mapping_cpu_entry+0x5a/0x100
[    0.000000]  trap_init+0x2c/0x7b
[    0.000000]  start_kernel+0x24c/0x497
[    0.000000]  secondary_startup_64+0xa5/0xb0

Cute, isn't it? And then further down the line it triplefaults of course.

#4 lore raw

Re: [PATCH 1/2] Undo the split of setup_cpu_entry_area

Quoted 28 lines · Thomas Gleixner
Quoted 12 lines · Thomas Gleixner
Quoted 6 lines · Andy Lutomirski

This is obviously a hack. Either the patch should be adjusted back to the version I sent or trap_init should forcibly initialize all PMDs by something like __set_fixmap(..., __mkpte(0)); or however it's spelled.

I split it because the whole thing crashed when I kept the loop you had because it tried to allocate stuff. Had no time to figure out why, so I went the lazy way of making it "work".

[    0.000000]  dump_stack+0x85/0xc5
[    0.000000]  warn_alloc+0x114/0x1c0
[    0.000000]  __alloc_pages_slowpath+0x1089/0x10d0
[    0.000000]  __alloc_pages_nodemask+0x2e8/0x370
[    0.000000]  __get_free_pages+0x10/0x40
[    0.000000]  kpti_shadow_pagetable_walk+0x2b2/0x3e0
[    0.000000]  kpti_add_user_map+0xfe/0x330
[    0.000000]  kpti_add_mapping_cpu_entry+0x5a/0x100
[    0.000000]  trap_init+0x2c/0x7b
[    0.000000]  start_kernel+0x24c/0x497
[    0.000000]  secondary_startup_64+0xa5/0xb0
Quoted 1 line

Cute, isn't it? And then further down the line it triplefaults of course.

Hmm. My second patch should make this go away, though.

#5 lore raw

Re: [PATCH 1/2] Undo the split of setup_cpu_entry_area

Quoted 28 lines · Andy Lutomirski
Quoted 28 lines · Thomas Gleixner
Quoted 12 lines · Thomas Gleixner
Quoted 6 lines · Andy Lutomirski

This is obviously a hack. Either the patch should be adjusted back to the version I sent or trap_init should forcibly initialize all PMDs by something like __set_fixmap(..., __mkpte(0)); or however it's spelled.

I split it because the whole thing crashed when I kept the loop you had because it tried to allocate stuff. Had no time to figure out why, so I went the lazy way of making it "work".

[    0.000000]  dump_stack+0x85/0xc5
[    0.000000]  warn_alloc+0x114/0x1c0
[    0.000000]  __alloc_pages_slowpath+0x1089/0x10d0
[    0.000000]  __alloc_pages_nodemask+0x2e8/0x370
[    0.000000]  __get_free_pages+0x10/0x40
[    0.000000]  kpti_shadow_pagetable_walk+0x2b2/0x3e0
[    0.000000]  kpti_add_user_map+0xfe/0x330
[    0.000000]  kpti_add_mapping_cpu_entry+0x5a/0x100
[    0.000000]  trap_init+0x2c/0x7b
[    0.000000]  start_kernel+0x24c/0x497
[    0.000000]  secondary_startup_64+0xa5/0xb0
Quoted 3 lines
Quoted 1 line

Cute, isn't it? And then further down the line it triplefaults of course.

Hmm. My second patch should make this go away, though.

Yes, it's all a mess and I'm cleaning it up. Fighting with the PEBS/BTS buffers right now.

Thanks,

tglx

author maintainer
#6 lore raw

PATCH 2/2 x86/kpti: Reference all cpu_entry_area pagetables in the usermode tables

In reply to Andy Lutomirski

We were manually configuring cpu_entry_area in the usermode tables. This was error-prone and wasted memory. (Not much memory, but still.) Instead, just reference the same pagetables.

This avoids needing to keep the KPTI code and the normal cpu_entry_area code in sync, since the KPTI code no longer cares what's in cpu_entry_area.

[This does *not* work on the current KPTI series. It requires that
all the kernelmode cpu_entry_tables are pre-allocated. That happens in the series as I submitted it, but tglx changed it for reasons that I haven't figured out.]

Signed-off-by Andy Lutomirski <[email protected]>
---
 arch/x86/include/asm/fixmap.h | 14 +++++---
 arch/x86/include/asm/kpti.h   |  8 +++--
 arch/x86/kernel/cpu/common.c  |  3 --
 arch/x86/mm/kpti.c            | 82 ++++++++++++++++++++++++++-----------------
 4 files changed, 64 insertions(+), 43 deletions(-)
arch/x86/include/asm/fixmap.h +9 -5
index 839addd1eaec..a630cd2861f7 100644
@@ -142,16 +142,20 @@ enum fixed_addresses {
 #ifdef CONFIG_PARAVIRT
 	FIX_PARAVIRT_BOOTMAP,
 #endif
-	FIX_TEXT_POKE1,	/* reserve 2 pages for text_poke() */
-	FIX_TEXT_POKE0, /* first page is last, because allocation is backward */
 #ifdef	CONFIG_X86_INTEL_MID
 	FIX_LNW_VRTC,
 #endif
-	/* Fixmap entries to remap the GDTs, one per processor. */
-	FIX_CPU_ENTRY_AREA_TOP,
+	FIX_TEXT_POKE1,	/* reserve 2 pages for text_poke() */
+	FIX_TEXT_POKE0, /* first page is last, because allocation is backward */
+
+	/*
+	 * Fixmap entries to remap the GDTs, one per processor.  Align
+	 * to a PMD boundary.
+	 */
+	FIX_CPU_ENTRY_AREA_TOP = round_up(FIX_TEXT_POKE0 + 1, PTRS_PER_PMD),
 	FIX_CPU_ENTRY_AREA_BOTTOM = FIX_CPU_ENTRY_AREA_TOP + (CPU_ENTRY_AREA_PAGES * NR_CPUS) - 1,
 
-	__end_of_permanent_fixed_addresses,
+	__end_of_permanent_fixed_addresses = round_up(FIX_CPU_ENTRY_AREA_BOTTOM + 1, PTRS_PER_PMD),
 
 	/*
 	 * 512 temporary boot-time mappings, used by early_ioremap(),
arch/x86/include/asm/kpti.h +5 -3
index 0c10e86ae3f8..df52cec2a53b 100644
@@ -1,5 +1,8 @@
 #ifndef _ASM_X86_KPTI_H
 #define _ASM_X86_KPTI_H
+
+#include <linux/init.h>
+
 /*
  * Copyright(c) 2017 Intel Corporation. All rights reserved.
  *

@@ -34,10 +37,9 @@ extern int kpti_add_mapping(unsigned long addr, unsigned long size,
 			      unsigned long flags);
 
 /**
- *  kpti_add_mapping_cpu_entry - map the cpu entry area
- *  @cpu: the CPU for which the entry area is being mapped
+ *  kpti_clone_cpu_entry_areas - clone cpu_entry_areas to the usermode tables
  */
-extern void kpti_add_mapping_cpu_entry(int cpu);
+extern void __init kpti_clone_cpu_entry_areas(void);
 
 /**
  *  kpti_remove_mapping - remove a kernel mapping from the userpage tables
arch/x86/kernel/cpu/common.c +0 -3
index 00697119f983..3dc814519c92 100644
@@ -606,9 +606,6 @@ void __init setup_cpu_entry_area(int cpu)
 				sizeof(struct debug_store) / PAGE_SIZE,
 				PAGE_KERNEL);
 #endif
-	/* CPU 0's mapping is done in kpti_init() */
-	if (cpu)
-		kpti_add_mapping_cpu_entry(cpu);
 }
 
 /* Load the original GDT from the per-cpu structure */
arch/x86/mm/kpti.c +50 -32

101 lines not on this page · raw message

Signature · 1 line
2.13.6
#7 lore raw

Re: [PATCH 2/2] x86/kpti: Reference all cpu_entry_area pagetables in the usermode tables

Quoted 4 lines · Andy Lutomirski

We were manually configuring cpu_entry_area in the usermode tables. This was error-prone and wasted memory. (Not much memory, but still.) Instead, just reference the same pagetables.

Quoted 8 lines

This avoids needing to keep the KPTI code and the normal cpu_entry_area code in sync, since the KPTI code no longer cares what's in cpu_entry_area.

[This does *not* work on the current KPTI series. It requires that
all the kernelmode cpu_entry_tables are pre-allocated. That happens in the series as I submitted it, but tglx changed it for reasons that I haven't figured out.]

As I said it crashed and burned for yet unknown reasons. I'll dig into that.

Thanks,

tglx