x86: disable IRQs during CR4 changes

PATCH In review Nadav Amit 4 participants 9 messages Nov 25, 2017

Subsystems (4)

x86 KVM x86 x86 MM EDAC-core

lore

Share & tools

lore

Nadav Amit

author

2017-11-25 03:44 UTC #0 lore raw

v2 cover x86: disable IRQs during CR4 changes

CR4 needs to be updated atomically with its shadow value, as CR4 updates are performed in read-modify-write fashion which are based on the shadow value. If CR4 is changed between the read and the write, CR4 might not be updated correctly.

For this to happen, CR4 needs to be rewritten by an interrupt handler. [Presumably, writes to CR4 take place while preemption is disabled, although due to the experience with CR3 - who knows.] CR4.PGD can be updated by an interrupt handler, but it is restored to its previous value, so it should not introduce a race. However, it seems that allowing CR4 updates without disabling IRQs may present a potential future bug.

Cc Andy Lutomirski <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc [email protected]

Cc Tony Luck <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

v1 -> v2:
- Break into two patches (Andy)
- Rename refactored function to __cr4_set() (Andy)

Nadav Amit (2):
x86: refactor CR4 setting and shadow write
x86: disable IRQs before changing CR4

 arch/x86/include/asm/mmu_context.h   |  4 ++--
 arch/x86/include/asm/tlbflush.h      | 40 +++++++++++++++++++++---------------
 arch/x86/include/asm/virtext.h       |  2 +-
 arch/x86/kernel/cpu/common.c         | 38 +++++++++++++++++++++++++---------
 arch/x86/kernel/cpu/mcheck/mce.c     |  5 ++++-
 arch/x86/kernel/cpu/mcheck/p5.c      |  6 +++++-
 arch/x86/kernel/cpu/mcheck/winchip.c |  5 ++++-
 arch/x86/kernel/fpu/init.c           |  2 +-
 arch/x86/kernel/fpu/xstate.c         |  4 ++--
 arch/x86/kernel/process.c            | 20 +++++++++++++-----
 arch/x86/kernel/reboot.c             |  2 +-
 arch/x86/kvm/vmx.c                   | 13 ++++++++++--
 arch/x86/mm/init.c                   |  6 +++++-
 13 files changed, 102 insertions(+), 45 deletions(-)

Signature · 1 line

2.14.1

Nadav Amit

author

2017-11-25 03:29 UTC #1 lore raw

PATCH v2 1/2 x86: refactor CR4 setting and shadow write

Refactor the write to CR4 and its shadow value. This is done in preparation for the addition of an assertion to check that IRQs are disabled during CR4 update.

No functional change.

Cc Andy Lutomirski <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc [email protected]

Signed-off-by Nadav Amit <[email protected]>

---
 arch/x86/include/asm/tlbflush.h | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

arch/x86/include/asm/tlbflush.h +11 ~~-13~~

index 509046cfa5ce..e736f7f0ba92 100644
@@ -173,17 +173,20 @@ static inline void cr4_init_shadow(void)
 	this_cpu_write(cpu_tlbstate.cr4, __read_cr4());
 }
 
+static inline void __cr4_set(unsigned long cr4)
+{
+	this_cpu_write(cpu_tlbstate.cr4, cr4);
+	__write_cr4(cr4);
+}
+
 /* Set in this cpu's CR4. */
 static inline void cr4_set_bits(unsigned long mask)
 {
 	unsigned long cr4;
 
 	cr4 = this_cpu_read(cpu_tlbstate.cr4);
-	if ((cr4 | mask) != cr4) {
-		cr4 |= mask;
-		this_cpu_write(cpu_tlbstate.cr4, cr4);
-		__write_cr4(cr4);
-	}
+	if ((cr4 | mask) != cr4)
+		__cr4_set(cr4 | mask);
 }
 
 /* Clear in this cpu's CR4. */

@@ -192,11 +195,8 @@ static inline void cr4_clear_bits(unsigned long mask)
 	unsigned long cr4;
 
 	cr4 = this_cpu_read(cpu_tlbstate.cr4);
-	if ((cr4 & ~mask) != cr4) {
-		cr4 &= ~mask;
-		this_cpu_write(cpu_tlbstate.cr4, cr4);
-		__write_cr4(cr4);
-	}
+	if ((cr4 & ~mask) != cr4)
+		__cr4_set(cr4 & ~mask);
 }
 
 static inline void cr4_toggle_bits(unsigned long mask)

@@ -204,9 +204,7 @@ static inline void cr4_toggle_bits(unsigned long mask)
 	unsigned long cr4;
 
 	cr4 = this_cpu_read(cpu_tlbstate.cr4);
-	cr4 ^= mask;
-	this_cpu_write(cpu_tlbstate.cr4, cr4);
-	__write_cr4(cr4);
+	__cr4_set(cr4 ^ mask);
 }
 
 /* Read the CR4 shadow. */

tip-bot for Nadav Amit

merge bot

2017-11-25 12:38 UTC #2 lore raw

[tip:x86/urgent] x86/tlb: Refactor CR4 setting and shadow write

x86/tlb: Refactor CR4 setting and shadow write

Commit-ID:  0c3292ca8025c5aef44dc389ac3a6bf4a325e0be
Gitweb:     https://git.kernel.org/tip/0c3292ca8025c5aef44dc389ac3a6bf4a325e0be
Author:     Nadav Amit <[email protected]>
AuthorDate: Fri, 24 Nov 2017 19:29:06 -0800
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Sat, 25 Nov 2017 13:28:43 +0100

x86/tlb: Refactor CR4 setting and shadow write

Refactor the write to CR4 and its shadow value. This is done in preparation for the addition of an assertion to check that IRQs are disabled during CR4 update.

No functional change.

Signed-off-by Nadav Amit <[email protected]>

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

Cc [email protected]

Cc Andy Lutomirski <[email protected]>

Cc [email protected]

Link https://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/include/asm/tlbflush.h | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

arch/x86/include/asm/tlbflush.h +11 ~~-13~~

index 509046c..e736f7f 100644
@@ -173,17 +173,20 @@ static inline void cr4_init_shadow(void)
 	this_cpu_write(cpu_tlbstate.cr4, __read_cr4());
 }
 
+static inline void __cr4_set(unsigned long cr4)
+{
+	this_cpu_write(cpu_tlbstate.cr4, cr4);
+	__write_cr4(cr4);
+}
+
 /* Set in this cpu's CR4. */
 static inline void cr4_set_bits(unsigned long mask)
 {
 	unsigned long cr4;
 
 	cr4 = this_cpu_read(cpu_tlbstate.cr4);
-	if ((cr4 | mask) != cr4) {
-		cr4 |= mask;
-		this_cpu_write(cpu_tlbstate.cr4, cr4);
-		__write_cr4(cr4);
-	}
+	if ((cr4 | mask) != cr4)
+		__cr4_set(cr4 | mask);
 }
 
 /* Clear in this cpu's CR4. */

@@ -192,11 +195,8 @@ static inline void cr4_clear_bits(unsigned long mask)
 	unsigned long cr4;
 
 	cr4 = this_cpu_read(cpu_tlbstate.cr4);
-	if ((cr4 & ~mask) != cr4) {
-		cr4 &= ~mask;
-		this_cpu_write(cpu_tlbstate.cr4, cr4);
-		__write_cr4(cr4);
-	}
+	if ((cr4 & ~mask) != cr4)
+		__cr4_set(cr4 & ~mask);
 }
 
 static inline void cr4_toggle_bits(unsigned long mask)

@@ -204,9 +204,7 @@ static inline void cr4_toggle_bits(unsigned long mask)
 	unsigned long cr4;
 
 	cr4 = this_cpu_read(cpu_tlbstate.cr4);
-	cr4 ^= mask;
-	this_cpu_write(cpu_tlbstate.cr4, cr4);
-	__write_cr4(cr4);
+	__cr4_set(cr4 ^ mask);
 }
 
 /* Read the CR4 shadow. */

Nadav Amit

author

2017-11-25 03:29 UTC #3 lore raw

PATCH v2 2/2 x86: disable IRQs before changing CR4

In reply to Nadav Amit

CR4 changes need to be performed while IRQs are disabled in order to update the CR4 shadow and the actual register atomically. Actually, they are needed regardless of CR4 shadowing, since CR4 are performed in a read-modify-write manner.

Currently, however, this is not the case, as can be experienced by adding warnings when CR4 updates are performed and interrupts are enabled. It also appears that CR4 changes with enabled interrupts can be triggered by the user (PR_SET_TSC).

If CR4 updates are done while interrupts are enabled, an interrupt can be delivered between the CR4 read and the corresponding write of the modified value to CR4. If the interrupt handler changes CR4, the write would ignore the modified value.

It is not clear there are currently interrupt handlers that modify CR4 and do not restore immediately the original value, but there is an interrupt handler that changes CR4, when global PTEs are invalidated. Moreover, a recent patch considered doing change CR4 inside the interrupt handler, emphasizing that the current scheme is error-prone.

Prevent the issue by: adding a debug warning if CR4 is updated with enabled interrupts; changing CR4 manipulation function names to reflect the fact they need disabled IRQs and fix the callers.

Note that in some cases, e.g., kvm_cpu_vmxon(), it appears that saving and restoring the IRQs could have been spared. Yet, since these calls do not appear to be on the hot path, save and restore the IRQs to be on the safe side.

Cc Andy Lutomirski <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc [email protected]

Cc Tony Luck <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc "Radim Krčmář" <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 arch/x86/include/asm/mmu_context.h   |  4 ++--
 arch/x86/include/asm/tlbflush.h      | 16 +++++++++++----
 arch/x86/include/asm/virtext.h       |  2 +-
 arch/x86/kernel/cpu/common.c         | 38 ++++++++++++++++++++++++++----------
 arch/x86/kernel/cpu/mcheck/mce.c     |  5 ++++-
 arch/x86/kernel/cpu/mcheck/p5.c      |  6 +++++-
 arch/x86/kernel/cpu/mcheck/winchip.c |  5 ++++-
 arch/x86/kernel/fpu/init.c           |  2 +-
 arch/x86/kernel/fpu/xstate.c         |  4 ++--
 arch/x86/kernel/process.c            | 20 ++++++++++++++-----
 arch/x86/kernel/reboot.c             |  2 +-
 arch/x86/kvm/vmx.c                   | 13 ++++++++++--
 arch/x86/mm/init.c                   |  6 +++++-
 13 files changed, 91 insertions(+), 32 deletions(-)

arch/x86/include/asm/mmu_context.h +2 -2

index 6699fc441644..637cbda77eda 100644
@@ -30,9 +30,9 @@ static inline void load_mm_cr4(struct mm_struct *mm)
 {
 	if (static_key_false(&rdpmc_always_available) ||
 	    atomic_read(&mm->context.perf_rdpmc_allowed))
-		cr4_set_bits(X86_CR4_PCE);
+		cr4_set_bits_irqs_off(X86_CR4_PCE);
 	else
-		cr4_clear_bits(X86_CR4_PCE);
+		cr4_clear_bits_irqs_off(X86_CR4_PCE);
 }
 #else
 static inline void load_mm_cr4(struct mm_struct *mm) {}

arch/x86/include/asm/tlbflush.h +12 -4

index e736f7f0ba92..6b94dcf8c500 100644
@@ -175,12 +175,15 @@ static inline void cr4_init_shadow(void)
 
 static inline void __cr4_set(unsigned long cr4)
 {
+#ifdef CONFIG_LOCKDEP
+	WARN_ON(!irqs_disabled());
+#endif
 	this_cpu_write(cpu_tlbstate.cr4, cr4);
 	__write_cr4(cr4);
 }
 
 /* Set in this cpu's CR4. */
-static inline void cr4_set_bits(unsigned long mask)
+static inline void cr4_set_bits_irqs_off(unsigned long mask)
 {
 	unsigned long cr4;

@@ -190,7 +193,7 @@ static inline void cr4_set_bits(unsigned long mask)
 }
 
 /* Clear in this cpu's CR4. */
-static inline void cr4_clear_bits(unsigned long mask)
+static inline void cr4_clear_bits_irqs_off(unsigned long mask)
 {
 	unsigned long cr4;

@@ -199,7 +202,7 @@ static inline void cr4_clear_bits(unsigned long mask)
 		__cr4_set(cr4 & ~mask);
 }
 
-static inline void cr4_toggle_bits(unsigned long mask)
+static inline void cr4_toggle_bits_irqs_off(unsigned long mask)
 {
 	unsigned long cr4;

@@ -224,10 +227,15 @@ extern u32 *trampoline_cr4_features;
 
 static inline void cr4_set_bits_and_update_boot(unsigned long mask)
 {
+	unsigned long flags;
+
 	mmu_cr4_features |= mask;
 	if (trampoline_cr4_features)
 		*trampoline_cr4_features = mmu_cr4_features;
-	cr4_set_bits(mask);
+
+	local_irq_save(flags);
+	cr4_set_bits_irqs_off(mask);
+	local_irq_restore(flags);
 }
 
 extern void initialize_tlbstate_and_flush(void);

arch/x86/include/asm/virtext.h +1 -1

index 0116b2ee9e64..b403ca417b7d 100644
@@ -41,7 +41,7 @@ static inline int cpu_has_vmx(void)
 static inline void cpu_vmxoff(void)
 {
 	asm volatile (ASM_VMX_VMXOFF : : : "cc");
-	cr4_clear_bits(X86_CR4_VMXE);
+	cr4_clear_bits_irqs_off(X86_CR4_VMXE);
 }
 
 static inline int cpu_vmx_enabled(void)

arch/x86/kernel/cpu/common.c +28 ~~-10~~

index c9176bae7fd8..f31890787d01 100644
@@ -302,8 +302,14 @@ __setup("nosmep", setup_disable_smep);
 
 static __always_inline void setup_smep(struct cpuinfo_x86 *c)
 {
-	if (cpu_has(c, X86_FEATURE_SMEP))
-		cr4_set_bits(X86_CR4_SMEP);
+	unsigned long flags;
+
+	if (!cpu_has(c, X86_FEATURE_SMEP))
+		return;
+
+	local_irq_save(flags);
+	cr4_set_bits_irqs_off(X86_CR4_SMEP);
+	local_irq_restore(flags);
 }
 
 static __init int setup_disable_smap(char *arg)

@@ -320,13 +326,16 @@ static __always_inline void setup_smap(struct cpuinfo_x86 *c)
 	/* This should have been cleared long ago */
 	BUG_ON(eflags & X86_EFLAGS_AC);
 
-	if (cpu_has(c, X86_FEATURE_SMAP)) {
+	if (!cpu_has(c, X86_FEATURE_SMAP))
+		return;
+
+	local_irq_save(eflags);
 #ifdef CONFIG_X86_SMAP
-		cr4_set_bits(X86_CR4_SMAP);
+	cr4_set_bits_irqs_off(X86_CR4_SMAP);
 #else
-		cr4_clear_bits(X86_CR4_SMAP);
+	cr4_clear_bits_irqs_off(X86_CR4_SMAP);
 #endif
-	}
+	local_irq_restore(eflags);
 }
 
 /*

@@ -336,6 +345,8 @@ static bool pku_disabled;
 
 static __always_inline void setup_pku(struct cpuinfo_x86 *c)
 {
+	unsigned long flags;
+
 	/* check the boot processor, plus compile options for PKU: */
 	if (!cpu_feature_enabled(X86_FEATURE_PKU))
 		return;

@@ -345,7 +356,10 @@ static __always_inline void setup_pku(struct cpuinfo_x86 *c)
 	if (pku_disabled)
 		return;
 
-	cr4_set_bits(X86_CR4_PKE);
+	local_irq_save(flags);
+	cr4_set_bits_irqs_off(X86_CR4_PKE);
+	local_irq_restore(flags);
+
 	/*
 	 * Seting X86_CR4_PKE will cause the X86_FEATURE_OSPKE
 	 * cpuid bit to be set.  We need to ensure that we

@@ -1147,7 +1161,10 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 	/* Disable the PN if appropriate */
 	squash_the_stupid_serial_number(c);
 
-	/* Set up SMEP/SMAP */
+	/*
+	 * Set up SMEP/SMAP. Disable interrupts to prevent triggering a warning
+	 * as CR4 changes must be done with disabled interrupts.
+	 */
 	setup_smep(c);
 	setup_smap(c);

@@ -1520,7 +1537,7 @@ void cpu_init(void)
 
 	pr_debug("Initializing CPU#%d\n", cpu);
 
-	cr4_clear_bits(X86_CR4_VME|X86_CR4_PVI|X86_CR4_TSD|X86_CR4_DE);
+	cr4_clear_bits_irqs_off(X86_CR4_VME|X86_CR4_PVI|X86_CR4_TSD|X86_CR4_DE);
 
 	/*
 	 * Initialize the per-CPU GDT with the boot GDT,

@@ -1613,7 +1630,8 @@ void cpu_init(void)
 	if (cpu_feature_enabled(X86_FEATURE_VME) ||
 	    boot_cpu_has(X86_FEATURE_TSC) ||
 	    boot_cpu_has(X86_FEATURE_DE))
-		cr4_clear_bits(X86_CR4_VME|X86_CR4_PVI|X86_CR4_TSD|X86_CR4_DE);
+		cr4_clear_bits_irqs_off(X86_CR4_VME|X86_CR4_PVI|X86_CR4_TSD|
+					X86_CR4_DE);
 
 	load_current_idt();
 	switch_to_new_gdt(cpu);

arch/x86/kernel/cpu/mcheck/mce.c +4 -1

index 3b413065c613..1aac196eb145 100644
@@ -1508,6 +1508,7 @@ static void __mcheck_cpu_init_generic(void)
 {
 	enum mcp_flags m_fl = 0;
 	mce_banks_t all_banks;
+	unsigned long flags;
 	u64 cap;
 
 	if (!mca_cfg.bootlog)

@@ -1519,7 +1520,9 @@ static void __mcheck_cpu_init_generic(void)
 	bitmap_fill(all_banks, MAX_NR_BANKS);
 	machine_check_poll(MCP_UC | m_fl, &all_banks);
 
-	cr4_set_bits(X86_CR4_MCE);
+	local_irq_save(flags);
+	cr4_set_bits_irqs_off(X86_CR4_MCE);
+	local_irq_restore(flags);
 
 	rdmsrl(MSR_IA32_MCG_CAP, cap);
 	if (cap & MCG_CTL_P)

arch/x86/kernel/cpu/mcheck/p5.c +5 -1

index 5cddf831720f..e4bb9573cfe5 100644
@@ -43,6 +43,7 @@ static void pentium_machine_check(struct pt_regs *regs, long error_code)
 /* Set up machine check reporting for processors with Intel style MCE: */
 void intel_p5_mcheck_init(struct cpuinfo_x86 *c)
 {
+	unsigned long flags;
 	u32 l, h;
 
 	/* Default P5 to off as its often misconnected: */

@@ -63,7 +64,10 @@ void intel_p5_mcheck_init(struct cpuinfo_x86 *c)
 	pr_info("Intel old style machine check architecture supported.\n");
 
 	/* Enable MCE: */
-	cr4_set_bits(X86_CR4_MCE);
+	local_irq_save(flags);
+	cr4_set_bits_irqs_off(X86_CR4_MCE);
+	local_irq_restore(flags);
+
 	pr_info("Intel old style machine check reporting enabled on CPU#%d.\n",
 		smp_processor_id());
 }

arch/x86/kernel/cpu/mcheck/winchip.c +4 -1

index 3b45b270a865..72213d75c865 100644
@@ -27,6 +27,7 @@ static void winchip_machine_check(struct pt_regs *regs, long error_code)
 /* Set up machine check reporting on the Winchip C6 series */
 void winchip_mcheck_init(struct cpuinfo_x86 *c)
 {
+	unsigned long flags;
 	u32 lo, hi;
 
 	machine_check_vector = winchip_machine_check;

@@ -38,7 +39,9 @@ void winchip_mcheck_init(struct cpuinfo_x86 *c)
 	lo &= ~(1<<4);	/* Enable MCE */
 	wrmsr(MSR_IDT_FCR1, lo, hi);
 
-	cr4_set_bits(X86_CR4_MCE);
+	local_irq_save(flags);
+	cr4_set_bits_irqs_off(X86_CR4_MCE);
+	local_irq_restore(flags);
 
 	pr_info("Winchip machine check reporting enabled on CPU#0.\n");
 }

arch/x86/kernel/fpu/init.c +1 -1

index 7affb7e3d9a5..db57b217e123 100644
@@ -23,7 +23,7 @@ static void fpu__init_cpu_generic(void)
 	if (boot_cpu_has(X86_FEATURE_XMM))
 		cr4_mask |= X86_CR4_OSXMMEXCPT;
 	if (cr4_mask)
-		cr4_set_bits(cr4_mask);
+		cr4_set_bits_irqs_off(cr4_mask);
 
 	cr0 = read_cr0();
 	cr0 &= ~(X86_CR0_TS|X86_CR0_EM); /* clear TS and EM */

arch/x86/kernel/fpu/xstate.c +2 -2

index f1d5476c9022..9d3c7a1a4ce5 100644
@@ -237,7 +237,7 @@ void fpu__init_cpu_xstate(void)
 
 	xfeatures_mask &= ~XFEATURE_MASK_SUPERVISOR;
 
-	cr4_set_bits(X86_CR4_OSXSAVE);
+	cr4_set_bits_irqs_off(X86_CR4_OSXSAVE);
 	xsetbv(XCR_XFEATURE_ENABLED_MASK, xfeatures_mask);
 }

@@ -713,7 +713,7 @@ static int init_xstate_size(void)
 static void fpu__init_disable_system_xstate(void)
 {
 	xfeatures_mask = 0;
-	cr4_clear_bits(X86_CR4_OSXSAVE);
+	cr4_clear_bits_irqs_off(X86_CR4_OSXSAVE);
 	fpu__xstate_clear_all_cpu_caps();
 }

arch/x86/kernel/process.c +15 -5

index c67685337c5a..412265fe14df 100644
@@ -128,25 +128,35 @@ void flush_thread(void)
 
 void disable_TSC(void)
 {
+	unsigned long flags;
+
 	preempt_disable();
-	if (!test_and_set_thread_flag(TIF_NOTSC))
+	if (!test_and_set_thread_flag(TIF_NOTSC)) {
 		/*
 		 * Must flip the CPU state synchronously with
 		 * TIF_NOTSC in the current running context.
 		 */
-		cr4_set_bits(X86_CR4_TSD);
+		local_irq_save(flags);
+		cr4_set_bits_irqs_off(X86_CR4_TSD);
+		local_irq_restore(flags);
+	}
 	preempt_enable();
 }
 
 static void enable_TSC(void)
 {
+	unsigned long flags;
+
 	preempt_disable();
-	if (test_and_clear_thread_flag(TIF_NOTSC))
+	if (test_and_clear_thread_flag(TIF_NOTSC)) {
 		/*
 		 * Must flip the CPU state synchronously with
 		 * TIF_NOTSC in the current running context.
 		 */
-		cr4_clear_bits(X86_CR4_TSD);
+		local_irq_save(flags);
+		cr4_clear_bits_irqs_off(X86_CR4_TSD);
+		local_irq_restore(flags);
+	}
 	preempt_enable();
 }

@@ -293,7 +303,7 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 	}
 
 	if ((tifp ^ tifn) & _TIF_NOTSC)
-		cr4_toggle_bits(X86_CR4_TSD);
+		cr4_toggle_bits_irqs_off(X86_CR4_TSD);
 
 	if ((tifp ^ tifn) & _TIF_NOCPUID)
 		set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));

arch/x86/kernel/reboot.c +1 -1

index 2126b9d27c34..86ad70c02607 100644
@@ -109,7 +109,7 @@ void __noreturn machine_real_restart(unsigned int type)
 
 	/* Exiting long mode will fail if CR4.PCIDE is set. */
 	if (static_cpu_has(X86_FEATURE_PCID))
-		cr4_clear_bits(X86_CR4_PCIDE);
+		cr4_clear_bits_irqs_off(X86_CR4_PCIDE);
 #endif
 
 	/* Jump to the identity-mapped low memory code */

arch/x86/kvm/vmx.c +11 -2

index a6f4f095f8f4..a0b387dfbf5c 100644
@@ -3500,7 +3500,12 @@ static __init int vmx_disabled_by_bios(void)
 
 static void kvm_cpu_vmxon(u64 addr)
 {
-	cr4_set_bits(X86_CR4_VMXE);
+	unsigned long flags;
+
+	local_irq_save(flags);
+	cr4_set_bits_irqs_off(X86_CR4_VMXE);
+	local_irq_restore(flags);
+
 	intel_pt_handle_vmx(1);
 
 	asm volatile (ASM_VMX_VMXON_RAX

@@ -3565,10 +3570,14 @@ static void vmclear_local_loaded_vmcss(void)
  */
 static void kvm_cpu_vmxoff(void)
 {
+	unsigned long flags;
+
 	asm volatile (__ex(ASM_VMX_VMXOFF) : : : "cc");
 
 	intel_pt_handle_vmx(0);
-	cr4_clear_bits(X86_CR4_VMXE);
+	local_irq_save(flags);
+	cr4_clear_bits_irqs_off(X86_CR4_VMXE);
+	local_irq_restore(flags);
 }
 
 static void hardware_disable(void)

arch/x86/mm/init.c +5 -1

index af5c1ed21d43..ddd248acab8e 100644
@@ -199,6 +199,8 @@ static void setup_pcid(void)
 #ifdef CONFIG_X86_64
 	if (boot_cpu_has(X86_FEATURE_PCID)) {
 		if (boot_cpu_has(X86_FEATURE_PGE)) {
+			unsigned long flags;
+
 			/*
 			 * This can't be cr4_set_bits_and_update_boot() --
 			 * the trampoline code can't handle CR4.PCIDE and

@@ -210,7 +212,9 @@ static void setup_pcid(void)
 			 * Instead, we brute-force it and set CR4.PCIDE
 			 * manually in start_secondary().
 			 */
-			cr4_set_bits(X86_CR4_PCIDE);
+			local_irq_save(flags);
+			cr4_set_bits_irqs_off(X86_CR4_PCIDE);
+			local_irq_restore(flags);
 		} else {
 			/*
 			 * flush_tlb_all(), as currently implemented, won't

Thomas Gleixner

2017-11-25 10:36 UTC #4 lore raw

Re: [PATCH v2 2/2] x86: disable IRQs before changing CR4

arch/x86/include/asm/tlbflush.h static inline void cr4_init_shadow(void) · line 186

Quoted 3 lines · Nadav Amit

/* Set in this cpu's CR4. */
arch/x86/include/asm/tlbflush.h +1 -1
-static inline void cr4_set_bits(unsigned long mask)
+static inline void cr4_set_bits_irqs_off(unsigned long mask)

This change is kinda weird. I'd expect that there is a corresponding function cr4_set_bits() which takes care of disabling interrupts. But there is not. All it does is creating a lot of pointless churn.

Quoted 3 lines

static __always_inline void setup_smep(struct cpuinfo_x86 *c) static __init int setup_disable_smap(char *arg) static __always_inline void setup_pku(struct cpuinfo_x86 *c)

Why are you not doing this at the call site around all calls which fiddle with cr4, i.e. in identify_cpu() ?

identify_cpu() is called from two places:

identify_boot_cpu() and identify_secondary_cpu()

identify_secondary_cpu is called with interrupts disabled anyway and there is no reason why we can't enforce interrupts being disabled around identify_cpu() completely.

But if we actually do the right thing, i.e. having cr4_set_bit() and cr4_set_bit_irqsoff() all of this churn goes away magically.

Then the only place which needs to be changed is the context switch because here interrupts are already disabled and we really care about performance.

arch/x86/kernel/process.c void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p, · line 307

Quoted 7 lines

arch/x86/kernel/process.c +1 -1

@@ -293,7 +303,7 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 	}
 
 	if ((tifp ^ tifn) & _TIF_NOTSC)
-		cr4_toggle_bits(X86_CR4_TSD);
+		cr4_toggle_bits_irqs_off(X86_CR4_TSD);

Thanks,

tglx
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Nadav Amit

2017-11-25 17:20 UTC #5 lore raw

Re: [PATCH v2 2/2] x86: disable IRQs before changing CR4

Quoted 30 lines · Thomas Gleixner

arch/x86/include/asm/tlbflush.h static inline void cr4_init_shadow(void) · line 186
Quoted 3 lines · Nadav Amit
/* Set in this cpu's CR4. */
arch/x86/include/asm/tlbflush.h +1 -1
-static inline void cr4_set_bits(unsigned long mask)
+static inline void cr4_set_bits_irqs_off(unsigned long mask)
This change is kinda weird. I'd expect that there is a corresponding function cr4_set_bits() which takes care of disabling interrupts. But there is not. All it does is creating a lot of pointless churn.
Quoted 3 lines
static __always_inline void setup_smep(struct cpuinfo_x86 *c) static __init int setup_disable_smap(char *arg) static __always_inline void setup_pku(struct cpuinfo_x86 *c)
Why are you not doing this at the call site around all calls which fiddle with cr4, i.e. in identify_cpu() ?
identify_cpu() is called from two places:
identify_boot_cpu() and identify_secondary_cpu()
identify_secondary_cpu is called with interrupts disabled anyway and there is no reason why we can't enforce interrupts being disabled around identify_cpu() completely.
But if we actually do the right thing, i.e. having cr4_set_bit() and cr4_set_bit_irqsoff() all of this churn goes away magically.
Then the only place which needs to be changed is the context switch because here interrupts are already disabled and we really care about performance.

Quoted 6 lines

arch/x86/kernel/process.c void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p, · line 306
Quoted 6 lines
arch/x86/kernel/process.c +1 -1
@@ -293,7 +303,7 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 	}
 
 	if ((tifp ^ tifn) & _TIF_NOTSC)
-		cr4_toggle_bits(X86_CR4_TSD);
+		cr4_toggle_bits_irqs_off(X86_CR4_TSD);

You make a good point. I will add cr4_set_bit(). I will leave identify_cpu() as is, since it is rather hard to maintain code that enables/disables irqs at one point and rely on these operations at a completely different place. As you said, it is less of an issue once cr4_set_bit() and friends are introduced.

Thanks,
Nadav--
To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thomas Gleixner

2017-11-25 17:25 UTC #6 lore raw

Re: [PATCH v2 2/2] x86: disable IRQs before changing CR4

Quoted 35 lines · Nadav Amit

Quoted 33 lines · Thomas Gleixner
arch/x86/include/asm/tlbflush.h static inline void cr4_init_shadow(void) · line 186
Quoted 3 lines · Nadav Amit
/* Set in this cpu's CR4. */
arch/x86/include/asm/tlbflush.h +1 -1
-static inline void cr4_set_bits(unsigned long mask)
+static inline void cr4_set_bits_irqs_off(unsigned long mask)
This change is kinda weird. I'd expect that there is a corresponding function cr4_set_bits() which takes care of disabling interrupts. But there is not. All it does is creating a lot of pointless churn.
Quoted 3 lines
static __always_inline void setup_smep(struct cpuinfo_x86 *c) static __init int setup_disable_smap(char *arg) static __always_inline void setup_pku(struct cpuinfo_x86 *c)
Why are you not doing this at the call site around all calls which fiddle with cr4, i.e. in identify_cpu() ?
identify_cpu() is called from two places:
identify_boot_cpu() and identify_secondary_cpu()
identify_secondary_cpu is called with interrupts disabled anyway and there is no reason why we can't enforce interrupts being disabled around identify_cpu() completely.
But if we actually do the right thing, i.e. having cr4_set_bit() and cr4_set_bit_irqsoff() all of this churn goes away magically.
Then the only place which needs to be changed is the context switch because here interrupts are already disabled and we really care about performance.
Quoted 3 lines
(quoted diff) +0 -0
@@ -293,7 +303,7 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 	}
 

Quoted 9 lines

Quoted 3 lines
arch/x86/kernel/process.c void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p, · line 306
Quoted 3 lines
arch/x86/kernel/process.c +1 -1
 	if ((tifp ^ tifn) & _TIF_NOTSC)
-		cr4_toggle_bits(X86_CR4_TSD);
+		cr4_toggle_bits_irqs_off(X86_CR4_TSD);
You make a good point. I will add cr4_set_bit(). I will leave identify_cpu() as is, since it is rather hard to maintain code that enables/disables irqs at one point and rely on these operations at a completely different place. As you said, it is less of an issue once cr4_set_bit() and friends are introduced.

I fixed that up already as I wanted to have it done, see the tip-bot mail in your inbox.

Thanks,

tglx
---
To unsubscribe from this list: send the line "unsubscribe linux-edac" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Nadav Amit

2017-11-25 17:31 UTC #7 lore raw

Re: [PATCH v2 2/2] x86: disable IRQs before changing CR4

Quoted 46 lines · Thomas Gleixner

Quoted 35 lines · Nadav Amit
Quoted 33 lines · Thomas Gleixner
arch/x86/include/asm/tlbflush.h static inline void cr4_init_shadow(void) · line 186
Quoted 3 lines · Nadav Amit
/* Set in this cpu's CR4. */
arch/x86/include/asm/tlbflush.h +1 -1
-static inline void cr4_set_bits(unsigned long mask)
+static inline void cr4_set_bits_irqs_off(unsigned long mask)
This change is kinda weird. I'd expect that there is a corresponding function cr4_set_bits() which takes care of disabling interrupts. But there is not. All it does is creating a lot of pointless churn.
Quoted 3 lines
static __always_inline void setup_smep(struct cpuinfo_x86 *c) static __init int setup_disable_smap(char *arg) static __always_inline void setup_pku(struct cpuinfo_x86 *c)
Why are you not doing this at the call site around all calls which fiddle with cr4, i.e. in identify_cpu() ?
identify_cpu() is called from two places:
identify_boot_cpu() and identify_secondary_cpu()
identify_secondary_cpu is called with interrupts disabled anyway and there is no reason why we can't enforce interrupts being disabled around identify_cpu() completely.
But if we actually do the right thing, i.e. having cr4_set_bit() and cr4_set_bit_irqsoff() all of this churn goes away magically.
Then the only place which needs to be changed is the context switch because here interrupts are already disabled and we really care about performance.
Quoted 3 lines
(quoted diff) +0 -0
@@ -293,7 +303,7 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 	}
 
Quoted 9 lines
Quoted 3 lines
arch/x86/kernel/process.c void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p, · line 306
Quoted 3 lines
arch/x86/kernel/process.c +1 -1
 	if ((tifp ^ tifn) & _TIF_NOTSC)
-		cr4_toggle_bits(X86_CR4_TSD);
+		cr4_toggle_bits_irqs_off(X86_CR4_TSD);
You make a good point. I will add cr4_set_bit(). I will leave identify_cpu() as is, since it is rather hard to maintain code that enables/disables irqs at one point and rely on these operations at a completely different place. As you said, it is less of an issue once cr4_set_bit() and friends are introduced.

Quoted 2 lines

I fixed that up already as I wanted to have it done, see the tip-bot mail in your inbox.

Thanks, your changes made it much better. At some point it might be better to make the MTRR code to use these interfaces too instead of meddling with CR4 directly. Anyhow, that is a different story.

Regards,
Nadav

tip-bot for Nadav Amit

merge bot

2017-11-25 12:40 UTC #8 lore raw

[tip:x86/urgent] x86/tlb: Disable interrupts when changing CR4

In reply to Nadav Amit

x86/tlb: Disable interrupts when changing CR4

Commit-ID:  9d0b62328d34c7044114d4f4281981d4c537c4ba
Gitweb:     https://git.kernel.org/tip/9d0b62328d34c7044114d4f4281981d4c537c4ba
Author:     Nadav Amit <[email protected]>
AuthorDate: Fri, 24 Nov 2017 19:29:07 -0800
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Sat, 25 Nov 2017 13:28:43 +0100

x86/tlb: Disable interrupts when changing CR4

CR4 modifications are implemented as RMW operations which update a shadow variable and write the result to CR4. The RMW operation is protected by preemption disable, but there is no enforcement or debugging mechanism.

CR4 modifications happen also in interrupt context via __native_flush_tlb_global(). This implementation does not affect a interrupted thread context CR4 operation, because the CR4 toggle restores the original content and does not modify the shadow variable.

So the current situation seems to be safe, but a recent patch tried to add an actual RMW operation in interrupt context, which will cause subtle corruptions.

To prevent that and make the CR4 handling future proof:

- Add a lockdep assertion to __cr4_set() which will catch interrupt enabled invocations

- Disable interrupts in the cr4 manipulator inlines

- Rename cr4_toggle_bits() to cr4_toggle_bits_irqsoff(). This is called from __switch_to_xtra() where interrupts are already disabled and performance matters.

All other call sites are not performance critical, so the extra overhead of an additional local_irq_save/restore() pair is not a problem. If new call sites care about performance then the necessary _irqsoff() variants can be added.

[ tglx: Condensed the patch by moving the irq protection inside the
manipulator functions. Updated changelog ]

Signed-off-by Nadav Amit <[email protected]>

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

Cc Luck <[email protected]>

Cc Radim Krčmář <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Paolo Bonzini <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc [email protected]

Link https://lkml.kernel.org/r/[email protected] open here

---
 arch/x86/include/asm/tlbflush.h | 11 ++++++++---
 arch/x86/kernel/process.c       |  2 +-
 2 files changed, 9 insertions(+), 4 deletions(-)

arch/x86/include/asm/tlbflush.h +8 -3

index e736f7f..877b5c1 100644
@@ -175,6 +175,7 @@ static inline void cr4_init_shadow(void)
 
 static inline void __cr4_set(unsigned long cr4)
 {
+	lockdep_assert_irqs_disabled();
 	this_cpu_write(cpu_tlbstate.cr4, cr4);
 	__write_cr4(cr4);
 }

@@ -182,24 +183,28 @@ static inline void __cr4_set(unsigned long cr4)
 /* Set in this cpu's CR4. */
 static inline void cr4_set_bits(unsigned long mask)
 {
-	unsigned long cr4;
+	unsigned long cr4, flags;
 
+	local_irq_save(flags);
 	cr4 = this_cpu_read(cpu_tlbstate.cr4);
 	if ((cr4 | mask) != cr4)
 		__cr4_set(cr4 | mask);
+	local_irq_restore(flags);
 }
 
 /* Clear in this cpu's CR4. */
 static inline void cr4_clear_bits(unsigned long mask)
 {
-	unsigned long cr4;
+	unsigned long cr4, flags;
 
+	local_irq_save(flags);
 	cr4 = this_cpu_read(cpu_tlbstate.cr4);
 	if ((cr4 & ~mask) != cr4)
 		__cr4_set(cr4 & ~mask);
+	local_irq_restore(flags);
 }
 
-static inline void cr4_toggle_bits(unsigned long mask)
+static inline void cr4_toggle_bits_irqsoff(unsigned long mask)
 {
 	unsigned long cr4;

arch/x86/kernel/process.c +1 -1

index 97fb3e5..bb988a2 100644
@@ -299,7 +299,7 @@ void __switch_to_xtra(struct task_struct *prev_p, struct task_struct *next_p,
 	}
 
 	if ((tifp ^ tifn) & _TIF_NOTSC)
-		cr4_toggle_bits(X86_CR4_TSD);
+		cr4_toggle_bits_irqsoff(X86_CR4_TSD);
 
 	if ((tifp ^ tifn) & _TIF_NOCPUID)
 		set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));