x86/entry: Consolidation - Part I

All exception entry points must have ASM_CLAC right at the beginning. The general_protection entry is missing one.

Fixes e59d1b0a2419 ("x86-32, smap: Add STAC/CLAC instructions to 32-bit kernel entry")

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

Cc [email protected]

From: Andy Lutomirski <[email protected]>

do_machine_check() can be raised in almost any context including the most fragile ones. Prevent kprobes and tracing.

Signed-off-by Andy Lutomirski <[email protected]>

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

---
 arch/x86/include/asm/traps.h   |    3 ---
 arch/x86/kernel/cpu/mce/core.c |   12 ++++++++++--
 2 files changed, 10 insertions(+), 5 deletions(-)

arch/x86/include/asm/traps.h +0 -3

@@ -88,9 +88,6 @@ dotraplinkage void do_page_fault(struct
 dotraplinkage void do_spurious_interrupt_bug(struct pt_regs *regs, long error_code);
 dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code);
 dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code);
-#ifdef CONFIG_X86_MCE
-dotraplinkage void do_machine_check(struct pt_regs *regs, long error_code);
-#endif
 dotraplinkage void do_simd_coprocessor_error(struct pt_regs *regs, long error_code);
 #ifdef CONFIG_X86_32
 dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code);

arch/x86/kernel/cpu/mce/core.c +7 -1

@@ -1213,8 +1213,14 @@ static void __mc_scan_banks(struct mce *
  * On Intel systems this is entered on all CPUs in parallel through
  * MCE broadcast. However some CPUs might be broken beyond repair,
  * so be always careful when synchronizing with others.
+ *
+ * Tracing and kprobes are disabled: if we interrupted a kernel context
+ * with IF=1, we need to minimize stack usage.  There are also recursion
+ * issues: if the machine check was due to a failure of the memory
+ * backing the user stack, tracing that reads the user stack will cause
+ * potentially infinite recursion.
  */
-void do_machine_check(struct pt_regs *regs, long error_code)
+void notrace do_machine_check(struct pt_regs *regs, long error_code)
 {
 	DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);

Quoted 42 lines · Thomas Gleixner

From: Andy Lutomirski <[email protected]>

do_machine_check() can be raised in almost any context including the most fragile ones. Prevent kprobes and tracing.

Signed-off-by Andy Lutomirski <[email protected]>

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

---
 arch/x86/include/asm/traps.h   |    3 ---
 arch/x86/kernel/cpu/mce/core.c |   12 ++++++++++--
 2 files changed, 10 insertions(+), 5 deletions(-)

arch/x86/include/asm/traps.h +0 -3

@@ -88,9 +88,6 @@ dotraplinkage void do_page_fault(struct
 dotraplinkage void do_spurious_interrupt_bug(struct pt_regs *regs, long error_code);
 dotraplinkage void do_coprocessor_error(struct pt_regs *regs, long error_code);
 dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code);
-#ifdef CONFIG_X86_MCE
-dotraplinkage void do_machine_check(struct pt_regs *regs, long error_code);
-#endif
 dotraplinkage void do_simd_coprocessor_error(struct pt_regs *regs, long error_code);
 #ifdef CONFIG_X86_32
 dotraplinkage void do_iret_error(struct pt_regs *regs, long error_code);

arch/x86/kernel/cpu/mce/core.c +7 -1

@@ -1213,8 +1213,14 @@ static void __mc_scan_banks(struct mce *
  * On Intel systems this is entered on all CPUs in parallel through
  * MCE broadcast. However some CPUs might be broken beyond repair,
  * so be always careful when synchronizing with others.
+ *
+ * Tracing and kprobes are disabled: if we interrupted a kernel context
+ * with IF=1, we need to minimize stack usage.  There are also recursion
+ * issues: if the machine check was due to a failure of the memory
+ * backing the user stack, tracing that reads the user stack will cause
+ * potentially infinite recursion.
  */
-void do_machine_check(struct pt_regs *regs, long error_code)
+void notrace do_machine_check(struct pt_regs *regs, long error_code)
 {
 	DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);

arch/x86/kernel/cpu/mce/core.c void do_machine_check(struct pt_regs *re · line 1369

Quoted 5 lines

arch/x86/kernel/cpu/mce/core.c +1 -0

@@ -1360,6 +1366,7 @@ void do_machine_check(struct pt_regs *re
 	ist_exit(regs);
 }
 EXPORT_SYMBOL_GPL(do_machine_check);
+NOKPROBE_SYMBOL(do_machine_check);

That won't protect all the function called by do_machine_check(), right? There are lots of them.

Quoted 4 lines

Quoted 4 lines

arch/x86/kernel/cpu/mce/core.c +1 -0

+void notrace do_machine_check(struct pt_regs *regs, long error_code)
 {
 	DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);

Quoted 9 lines

arch/x86/kernel/cpu/mce/core.c void do_machine_check(struct pt_regs *re · line 1369

Quoted 5 lines

arch/x86/kernel/cpu/mce/core.c +1 -0

@@ -1360,6 +1366,7 @@ void do_machine_check(struct pt_regs *re
 	ist_exit(regs);
 }
 EXPORT_SYMBOL_GPL(do_machine_check);
+NOKPROBE_SYMBOL(do_machine_check);

That won't protect all the function called by do_machine_check(), right? There are lots of them.

It at least means we can survive to run actual C code in do_machine_check(), which lets us try to mitigate this issue further. PeterZ has patches for that, and maybe this series fixes it later on. (I'm reading in order!)

Quoted 14 lines · Andy Lutomirski

Quoted 4 lines

Quoted 4 lines

+void notrace do_machine_check(struct pt_regs *regs, long error_code)

 {
   DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
   DECLARE_BITMAP(toclear, MAX_NR_BANKS);

Quoted 9 lines

arch/x86/kernel/cpu/mce/core.c void do_machine_check(struct pt_regs *re · line 1369

Quoted 5 lines

arch/x86/kernel/cpu/mce/core.c +1 -0

@@ -1360,6 +1366,7 @@ void do_machine_check(struct pt_regs *re
   ist_exit(regs);
 }
 EXPORT_SYMBOL_GPL(do_machine_check);
+NOKPROBE_SYMBOL(do_machine_check);

That won't protect all the function called by do_machine_check(), right? There are lots of them.

Quoted 4 lines

It at least means we can survive to run actual C code in do_machine_check(), which lets us try to mitigate this issue further. PeterZ has patches for that, and maybe this series fixes it later on. (I'm reading in order!)

Yeah, I don't cover that either. Making the kernel completely kprobe safe is _lots_ more work I think.

We really need some form of automation for this :/ The current situation is completely nonsatisfactory.

Quoted 24 lines · Peter Zijlstra

Quoted 14 lines · Andy Lutomirski

Quoted 4 lines

Quoted 4 lines

+void notrace do_machine_check(struct pt_regs *regs, long error_code)

 {
   DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
   DECLARE_BITMAP(toclear, MAX_NR_BANKS);

Quoted 9 lines

arch/x86/kernel/cpu/mce/core.c void do_machine_check(struct pt_regs *re · line 1369

Quoted 5 lines

arch/x86/kernel/cpu/mce/core.c +1 -0

@@ -1360,6 +1366,7 @@ void do_machine_check(struct pt_regs *re
   ist_exit(regs);
 }
 EXPORT_SYMBOL_GPL(do_machine_check);
+NOKPROBE_SYMBOL(do_machine_check);

That won't protect all the function called by do_machine_check(), right? There are lots of them.

Quoted 4 lines

It at least means we can survive to run actual C code in do_machine_check(), which lets us try to mitigate this issue further. PeterZ has patches for that, and maybe this series fixes it later on. (I'm reading in order!)

Yeah, I don't cover that either. Making the kernel completely kprobe safe is _lots_ more work I think.

Quoted 2 lines

We really need some form of automation for this :/ The current situation is completely nonsatisfactory.

I've looked at too many patches lately and lost track a bit of which is which. Shouldn't a simple tracing_disable() or similar in do_machine_check() be sufficient?

We'd maybe want automation to check
everything before it. We still need to survive hitting a kprobe int3, but that shouldn't have recursion issues.

Quoted 25 lines · Andy Lutomirski

Quoted 25 lines · Peter Zijlstra

Quoted 16 lines · Andy Lutomirski

Quoted 14 lines

arch/x86/kernel/cpu/mce/core.c void do_machine_check(struct pt_regs *re · line 1369

Quoted 10 lines

+void notrace do_machine_check(struct pt_regs *regs, long error_code) {
DECLARE_BITMAP(valid_banks, MAX_NR_BANKS); DECLARE_BITMAP(toclear, MAX_NR_BANKS);

arch/x86/kernel/cpu/mce/core.c +1 -0

@@ -1360,6 +1366,7 @@ void do_machine_check(struct pt_regs *re
  ist_exit(regs);
 }
 EXPORT_SYMBOL_GPL(do_machine_check);
+NOKPROBE_SYMBOL(do_machine_check);

That won't protect all the function called by do_machine_check(), right? There are lots of them.

Quoted 4 lines

It at least means we can survive to run actual C code in do_machine_check(), which lets us try to mitigate this issue further. PeterZ has patches for that, and maybe this series fixes it later on. (I'm reading in order!)

Yeah, I don't cover that either. Making the kernel completely kprobe safe is _lots_ more work I think.

Quoted 6 lines

Quoted 2 lines

We really need some form of automation for this :/ The current situation is completely nonsatisfactory.

I've looked at too many patches lately and lost track a bit of which is which. Shouldn't a simple tracing_disable() or similar in do_machine_check() be sufficient?

It entirely depends on what the goal is :-/ On the one hand I see why people might want function tracing / kprobes enabled, OTOH it's all mighty frigging scary. Any tracing/probing/whatever on an MCE has the potential to make a bad situation worse -- not unlike the same on #DF.

The same with that compiler instrumentation crap; allowing kprobes on *SAN code has the potential to inject probes in arbitrary random code. At the same time, if you're running a kernel with that on and injecting kprobes in it, you're welcome to own the remaining pieces.

How far do we want to go? At some point I think we'll have to give people rope, show then the knot and leave them be.

Quoted 4 lines

We'd maybe want automation to check
everything before it. We still need to survive hitting a kprobe int3, but that shouldn't have recursion issues.

Right, so I think avoiding the obvious recursion issues is a more tractable problem and yes some 'safe' spot annotation should be enough to get automation working for that -- mostly.

Quoted 4 lines

It entirely depends on what the goal is :-/ On the one hand I see why people might want function tracing / kprobes enabled, OTOH it's all mighty frigging scary. Any tracing/probing/whatever on an MCE has the potential to make a bad situation worse -- not unlike the same on #DF.

Quoted 6 lines · Borislav Petkov

Quoted 4 lines · Andy Lutomirski

Quoted 4 lines

It entirely depends on what the goal is :-/ On the one hand I see why people might want function tracing / kprobes enabled, OTOH it's all mighty frigging scary. Any tracing/probing/whatever on an MCE has the potential to make a bad situation worse -- not unlike the same on #DF.

Review

Quoted 6 lines

FWIW, I had this at the beginning of the #MC handler in a feeble attempt to poke at this:

(quoted diff) +3 -0

+       hw_breakpoint_disable();
+       static_key_disable(&__tracepoint_read_msr.key);
+       tracing_off();

You can't do static_key_disable() from an IST

Quoted 12 lines · Peter Zijlstra

Quoted 9 lines · Borislav Petkov

Quoted 4 lines · Andy Lutomirski

Quoted 4 lines

It entirely depends on what the goal is :-/ On the one hand I see why people might want function tracing / kprobes enabled, OTOH it's all mighty frigging scary. Any tracing/probing/whatever on an MCE has the potential to make a bad situation worse -- not unlike the same on #DF.

FWIW, I had this at the beginning of the #MC handler in a feeble attempt to poke at this:

Quoted 5 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+       hw_breakpoint_disable();
+       static_key_disable(&__tracepoint_read_msr.key);
+       tracing_off();  

You can't do static_key_disable() from an IST

Can we set a percpu variable saying “in some stupid context, don’t trace”?

Remove the pointless difference between 32 and 64 bit to make further unifications simpler.

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

From: Thomas Gleixner <[email protected]>

That function returns immediately after conditionally reenabling interrupts which is more than pointless and requires the ASM code to disable interrupts again.

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

Reviewed-by Sean Christopherson <[email protected]>

Reviewed-by Alexandre Chartre <[email protected]>

Acked-by Peter Zijlstra (Intel) <[email protected]>

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

Add a comment which explains why this empty handler for a reserved vector exists.

Requested-by Josh Poimboeuf <[email protected]>

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

---
 arch/x86/kernel/traps.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

arch/x86/kernel/traps.c +10 -0

@@ -862,6 +862,25 @@ do_simd_coprocessor_error(struct pt_regs
 dotraplinkage void
 do_spurious_interrupt_bug(struct pt_regs *regs, long error_code)
 {
+	/*
+	 * This addresses a Pentium Pro Erratum:
+	 *
+	 * PROBLEM: If the APIC subsystem is configured in mixed mode with
+	 * Virtual Wire mode implemented through the local APIC, an
+	 * interrupt vector of 0Fh (Intel reserved encoding) may be
+	 * generated by the local APIC (Int 15).  This vector may be
+	 * generated upon receipt of a spurious interrupt (an interrupt
+	 * which is removed before the system receives the INTA sequence)
+	 * instead of the programmed 8259 spurious interrupt vector.

Nothing cares about the -1 "mark as interrupt" in the errorcode anymore. Just use 0 for all excpetions which do not have an errorcode consistently.

Quoted 3 lines · Thomas Gleixner

Nothing cares about the -1 "mark as interrupt" in the errorcode anymore. Just use 0 for all excpetions which do not have an errorcode consistently.

I sincerely wish this were the case. But look at collect_syscall() in lib/syscall.c.

It would be really quite nice to address this for real in some low-overhead way. My suggestion would be to borrow a trick from 32-bit:
split regs->cs into ->cs and ->__csh, and stick CS_FROM_SYSCALL into __csh for syscalls. This will only add any overhead at all to the int80 case. Then we could adjust syscall_get_nr() to look for CS_FROM_SYSCALL.

What do you think? An alternative would be to use the stack walking machinery in collect_syscall(), since the mere existence of that function is abomination and we may not care about performance.

Quoted 14 lines · Andy Lutomirski

Quoted 3 lines · Thomas Gleixner

Nothing cares about the -1 "mark as interrupt" in the errorcode anymore. Just use 0 for all excpetions which do not have an errorcode consistently.

I sincerely wish this were the case. But look at collect_syscall() in lib/syscall.c.

It would be really quite nice to address this for real in some low-overhead way. My suggestion would be to borrow a trick from 32-bit:
split regs->cs into ->cs and ->__csh, and stick CS_FROM_SYSCALL into __csh for syscalls. This will only add any overhead at all to the int80 case. Then we could adjust syscall_get_nr() to look for CS_FROM_SYSCALL.

Quoted 3 lines

What do you think? An alternative would be to use the stack walking machinery in collect_syscall(), since the mere existence of that function is abomination and we may not care about performance.

Looking deeper. The code in common_exception does:

        movl    PT_ORIG_EAX(%esp), %edx         # get the error code
        movl    $-1, PT_ORIG_EAX(%esp)          # no syscall to restart

So whatever the exception pushed on the stack the thing what collect_syscall finds is -1.

The pushed value is used as the error_code argument for the exception handler and I really can't find a single one which cares (anymore).

But darn and I overlooked that, it's propagated to do_trap() and friends, but even if this causes a user visible change, I doubt that anything cares about it today simply because for giggles a 64bit kernel unconditionally pushes 0 for all exceptions which do not have a hardware error code on stack. So any 32bit application which excpects a particular error code (0/-1) in the signal would have been broken on the first day it ran on a x64 bit kernel.

If someone yells regression, then that's really trivial to fix in C-code.

Quoted 11 lines · Thomas Gleixner

The pushed value is used as the error_code argument for the exception handler and I really can't find a single one which cares (anymore).

But darn and I overlooked that, it's propagated to do_trap() and friends, but even if this causes a user visible change, I doubt that anything cares about it today simply because for giggles a 64bit kernel unconditionally pushes 0 for all exceptions which do not have a hardware error code on stack. So any 32bit application which excpects a particular error code (0/-1) in the signal would have been broken on the first day it ran on a x64 bit kernel.

Quoted 2 lines

If someone yells regression, then that's really trivial to fix in C-code.

I *think* this is plumbed much more directly to userspace:

$ cat /proc/$$/syscall
61 0xffffffff 0x7ffccf734ed0 0xa 0x0 0x1 0x0 0x7ffccf734eb8 0x7f0667465eda

That entire feature is highly dubious and I suppose we could just delete it. But right now, we at least pretend that we can tell, totally asynchronously, whether another task is in a syscall. Unless we do *something*, though, I think you shouldn't make this change.

int3 is not using the common_exception path for purely historical reasons, but there is no reason to keep it the only exception which is different.

Make it use common_exception so the upcoming changes to autogenerate the entry stubs do not have to special case int3.

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

Hi!

This is the first batch of a 73 patches series which consolidates the x86 entry code.

Hi!

This is the first batch of a 73 patches series which consolidates the x86 entry code.

Hi!

This is the first batch of a 73 patches series which consolidates the x86 entry code.

This work started off as a trivial 5 patches series moving the heavy lifting of POSIX CPU timers out of interrupt context into thread/process context. This discovered that KVM is lacking to handle pending work items before entering guest mode and added the handling to the x86 KVM code. Review requested to make this a generic infrastructure.

The next series grew to 25 patches implementing the generic infrastructure, converting x86 (and as a POC ARM64) over, but it turned out that this was slightly incomplete and still had some entanglement with the rest of the x86 entry code as some of that functionality is shared between syscall and interrupt entry/exit. And it also unearthed the nastyness of IOPL which got already addressed in mainline.

This series addresses these issues in order to prepare for making the entry from userspace and exit to userspace (and it's counterpart enter guest) a generic infrastructure in order to restrict the necessary ASM work to the bare minimum.

The series is split into 5 parts:

- General cleanups and bugfixes

- Consolidation of the syscall entry/exit code

- Autogenerate simple exception/trap code and reduce the difference between 32 and 64 bit

- Autogenerate complex exception/trap code and provide different entry points for #DB and #MC exceptions which allows to address the recently discovered RCU vs. world issues in a more structured way

- Convert the device interrupt entry code to use the same mechanism as exceptions and traps and finally convert the system vectors over as well. The last step after all those cleanups is to move the return from exception/interrupt logic (user mode work, kernel preemption) completely from ASM into C-code, so the ASM code just has to take care about returning from the exception, which is horrible and convoluted enough already.

At the end the x86 entry code is ready to move the syscall parts out into generic code and finally tackle the initial problem which started all of this.

The complete series is available from git:

git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git x86/entry

which contains all 73 patches. The individual parts are tagged, so this part can be retrieved via:

git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git entry-v1-part1

Thanks,

tglx

8<---------------

  entry/entry_32.S          |   19 +++++++------------
  include/asm/irq.h         |    2 +-
  include/asm/mce.h         |    3 ---
  include/asm/traps.h       |   17 +++++++----------
  kernel/cpu/mce/core.c     |   12 ++++++++++--
  kernel/cpu/mce/internal.h |    3 +++
  kernel/irq.c              |    3 +--
  kernel/traps.c            |   41 ++++++++++++++++++++++++++++++++++-------
  8 files changed, 63 insertions(+), 37 deletions(-)