Machine check recovery when kernel accesses poison

Copy the existing page fault fixup mechanisms to create a new table to be used when fixing machine checks. Note:
1) At this time we only provide a macro to annotate assembly code
2) We assume all fixups will in code builtin to the kernel.

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/include/asm/asm.h        |  7 +++++++
 arch/x86/include/asm/uaccess.h    |  1 +
 arch/x86/mm/extable.c             | 16 ++++++++++++++++
 include/asm-generic/vmlinux.lds.h |  6 ++++++
 include/linux/module.h            |  1 +
 kernel/extable.c                  | 14 ++++++++++++++
 6 files changed, 45 insertions(+)

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

index 189679aba703..f2fa7973f18f 100644

arch/x86/include/asm/asm.h +6 -0

+	.pushsection "__mcex_table", "a" ;			\
+	.balign 8 ;						\
+	.long (from) - . ;					\
+	.long (to) - . ;					\
+	.popsection
+
 # define _ASM_NOKPROBE(entry)					\
 	.pushsection "_kprobe_blacklist","aw" ;			\
 	_ASM_ALIGN ;						\

arch/x86/include/asm/uaccess.h +1 -0

index a8df874f3e88..b8231301a224 100644
@@ -111,6 +111,7 @@ struct exception_table_entry {
 #define ARCH_HAS_SEARCH_EXTABLE
 
 extern int fixup_exception(struct pt_regs *regs);
+extern int fixup_mcexception(struct pt_regs *regs);
 extern int early_fixup_exception(unsigned long *ip);
 
 /*

arch/x86/mm/extable.c +5 -0

index 903ec1e9c326..5b328ae00365 100644
@@ -49,6 +49,22 @@ int fixup_exception(struct pt_regs *regs)
 	return 0;
 }
 
+int fixup_mcexception(struct pt_regs *regs)
+{
+	const struct exception_table_entry *fixup;
+	unsigned long new_ip;
+

Quoted 1 line

+# define _ASM_MCEXTABLE(from, to) \

Maybe add an intermediary macro which abstracts the table name:

#define __ASM_EXTABLE(from, to, table)
...

and then do

#define _ASM_EXTABLE(from, to) __ASM_EXTABLE(from, to, "__ex_table")
#define _ASM_MCEXTABLE(from, to) __ASM_EXTABLE(from, to, "__mcex_table")

Yeah, all that duplication might raise some brows but I'd guess special-handling MCA in the normal exception paths might make the code a bit too ugly...

Copy the existing page fault fixup mechanisms to create a new table to be used when fixing machine checks. Note:
1) At this time we only provide a macro to annotate assembly code
2) We assume all fixups will in code builtin to the kernel.

Signed-off-by Tony Luck <[email protected]>

---
  arch/x86/include/asm/asm.h        |  7 +++++++
  arch/x86/include/asm/uaccess.h    |  1 +
  arch/x86/mm/extable.c             | 16 ++++++++++++++++
  include/asm-generic/vmlinux.lds.h |  6 ++++++
  include/linux/module.h            |  1 +
  kernel/extable.c                  | 14 ++++++++++++++
  6 files changed, 45 insertions(+)

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

index 189679aba703..f2fa7973f18f 100644

Quoted 4 lines · Tony Luck

Copy the existing page fault fixup mechanisms to create a new table to be used when fixing machine checks. Note:
1) At this time we only provide a macro to annotate assembly code
2) We assume all fixups will in code builtin to the kernel.

Shouldn't the first step be to fixup failures during user memory access?

This does something really weird to rax. (Also, what happens on 32-bit kernels? There's no bit 63.)

Please at least document it clearly.

Quoted 7 lines · Andy Lutomirski

Quoted 4 lines · Tony Luck

Copy the existing page fault fixup mechanisms to create a new table to be used when fixing machine checks. Note:
1) At this time we only provide a macro to annotate assembly code
2) We assume all fixups will in code builtin to the kernel.

Shouldn't the first step be to fixup failures during user memory access?

We already have code to recover from machine checks encountered while the processor is executing ring3 code.

Quoted 2 lines

We already have code to recover from machine checks encountered while the processor is executing ring3 code.

I meant failures during copy_from_user, copy_to_user, etc.

Extend the severity checking code to add a new context IN_KERN_RECOV which is used to indicate that the machine check was triggered by code in the kernel with a fixup entry.

Add code to check for this situation and respond by altering the return IP to the fixup address and changing the regs->ax so that the recovery code knows the physical address of the error. Note that we also set bit 63 because 0x0 is a legal physical address.

Signed-off-by Tony Luck <[email protected]>

---
 arch/x86/kernel/cpu/mcheck/mce-severity.c | 19 +++++++++++++++++--
 arch/x86/kernel/cpu/mcheck/mce.c          | 13 ++++++++++---
 2 files changed, 27 insertions(+), 5 deletions(-)

arch/x86/kernel/cpu/mcheck/mce.c +9 -3

 		if (no_way_out)
 			mce_panic("Fatal machine check on current CPU", &m, msg);
 		if (worst == MCE_AR_SEVERITY) {
-			recover_paddr = m.addr;
-			if (!(m.mcgstatus & MCG_STATUS_RIPV))
-				flags |= MF_MUST_KILL;
+			if ((m.cs & 3) == 3) {
+				recover_paddr = m.addr;
+				if (!(m.mcgstatus & MCG_STATUS_RIPV))
+					flags |= MF_MUST_KILL;
+			} else if (fixup_mcexception(regs)) {
+				regs->ax = BIT(63) | m.addr;
+			} else
+				mce_panic("Failed kernel mode recovery",
+					  &m, NULL);
 		} else if (kill_it) {
 			force_sig(SIGBUS, current);
 		}

Signature · 1 line

2.1.4

You could save a precious indentation level here:

	if (cfg->tolerant == 3)
		goto clear;

and add the "clear" label below.

clear:

        if (worst > 0)
                mce_report_event(regs);
        mce_wrmsrl(MSR_IA32_MCG_STATUS, 0)

Quoted 10 lines

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

 		if (no_way_out)
 			mce_panic("Fatal machine check on current CPU", &m, msg);
 		if (worst == MCE_AR_SEVERITY) {
-			recover_paddr = m.addr;
-			if (!(m.mcgstatus & MCG_STATUS_RIPV))
-				flags |= MF_MUST_KILL;
+			if ((m.cs & 3) == 3) {
+				recover_paddr = m.addr;
+				if (!(m.mcgstatus & MCG_STATUS_RIPV))
+					flags |= MF_MUST_KILL;

arch/x86/kernel/cpu/mcheck/mce.c void do_machine_check(struct pt_regs *regs, long error_code) · line 1147

Quoted 8 lines

arch/x86/kernel/cpu/mcheck/mce.c +5 -0

+			} else if (fixup_mcexception(regs)) {
+				regs->ax = BIT(63) | m.addr;
+			} else
+				mce_panic("Failed kernel mode recovery",
+					  &m, NULL);
 		} else if (kill_it) {
 			force_sig(SIGBUS, current);
 		}

That would be tidier ... the inside of the "if" has been gradually growing with added recovery paths. I had to fold the mce_panic() line to shut checkpatch up.

But I'm not really sure what tolerant==3 people really want here. By skipping the recovery code they doom themselves to hitting the machine check again.

Extend the severity checking code to add a new context IN_KERN_RECOV which is used to indicate that the machine check was triggered by code in the kernel with a fixup entry.

Add code to check for this situation and respond by altering the return IP to the fixup address and changing the regs->ax so that the recovery code knows the physical address of the error. Note that we also set bit 63 because 0x0 is a legal physical address.

Signed-off-by Tony Luck <[email protected]>

---
  arch/x86/kernel/cpu/mcheck/mce-severity.c | 19 +++++++++++++++++--
  arch/x86/kernel/cpu/mcheck/mce.c          | 13 ++++++++++---
  2 files changed, 27 insertions(+), 5 deletions(-)

arch/x86/kernel/cpu/mcheck/mce-severity.c +17 -2

index 9c682c222071..1e83842310e8 100644
@@ -12,6 +12,7 @@
  #include <linux/kernel.h>
  #include <linux/seq_file.h>
  #include <linux/init.h>
+#include <linux/module.h>
  #include <linux/debugfs.h>
  #include <asm/mce.h>
 

@@ -29,7 +30,7 @@
   * panic situations)
   */
 
-enum context { IN_KERNEL = 1, IN_USER = 2 };
+enum context { IN_KERNEL = 1, IN_USER = 2, IN_KERNEL_RECOV = 3 };
  enum ser { SER_REQUIRED = 1, NO_SER = 2 };
  enum exception { EXCP_CONTEXT = 1, NO_EXCP = 2 };
 

@@ -48,6 +49,7 @@ static struct severity {
  #define MCESEV(s, m, c...) { .sev = MCE_ ## s ## _SEVERITY, .msg = m, ## c }
  #define  KERNEL		.context = IN_KERNEL
  #define  USER		.context = IN_USER
+#define  KERNEL_RECOV	.context = IN_KERNEL_RECOV
  #define  SER		.ser = SER_REQUIRED
  #define  NOSER		.ser = NO_SER
  #define  EXCP		.excp = EXCP_CONTEXT

@@ -87,6 +89,10 @@ static struct severity {
  		EXCP, KERNEL, MCGMASK(MCG_STATUS_RIPV, 0)
  		),
  	MCESEV(
+		PANIC, "In kernel and no restart IP",
+		EXCP, KERNEL_RECOV, MCGMASK(MCG_STATUS_RIPV, 0)
+		),
+	MCESEV(
  		DEFERRED, "Deferred error",
  		NOSER, MASK(MCI_STATUS_UC|MCI_STATUS_DEFERRED|MCI_STATUS_POISON, MCI_STATUS_DEFERRED)
  		),

@@ -123,6 +129,11 @@ static struct severity {
  		MCGMASK(MCG_STATUS_RIPV|MCG_STATUS_EIPV, MCG_STATUS_RIPV)
  		),
  	MCESEV(
+		AR, "Action required: data load error recoverable area of kernel",
+		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_DATA),
+		KERNEL_RECOV
+		),
+	MCESEV(
  		AR, "Action required: data load error in a user process",
  		SER, MASK(MCI_STATUS_OVER|MCI_UC_SAR|MCI_ADDR|MCACOD, MCI_UC_SAR|MCI_ADDR|MCACOD_DATA),
  		USER

@@ -183,7 +194,11 @@ static struct severity {
   */
  static int error_context(struct mce *m)
  {
-	return ((m->cs & 3) == 3) ? IN_USER : IN_KERNEL;
+	if ((m->cs & 3) == 3)
+		return IN_USER;
+	if (search_mcexception_tables(m->ip))
+		return IN_KERNEL_RECOV;
+	return IN_KERNEL;
  }
 
  /*

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

index 9d014b82a124..472d11150b7a 100644
@@ -31,6 +31,7 @@
  #include <linux/types.h>
  #include <linux/slab.h>
  #include <linux/init.h>
+#include <linux/module.h>
  #include <linux/kmod.h>
  #include <linux/poll.h>
  #include <linux/nmi.h>

@@ -1132,9 +1133,15 @@ void do_machine_check(struct pt_regs *regs, long error_code)
  		if (no_way_out)
  			mce_panic("Fatal machine check on current CPU", &m, msg);
  		if (worst == MCE_AR_SEVERITY) {
-			recover_paddr = m.addr;
-			if (!(m.mcgstatus & MCG_STATUS_RIPV))

Quoted 12 lines

arch/x86/kernel/cpu/mcheck/mce.c +5 -3

@@ -1132,9 +1133,15 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 		if (no_way_out)
 			mce_panic("Fatal machine check on current CPU", &m, msg);
 		if (worst == MCE_AR_SEVERITY) {
-			recover_paddr = m.addr;
-			if (!(m.mcgstatus & MCG_STATUS_RIPV))
-				flags |= MF_MUST_KILL;
+			if ((m.cs & 3) == 3) {
+				recover_paddr = m.addr;
+				if (!(m.mcgstatus & MCG_STATUS_RIPV))
+					flags |= MF_MUST_KILL;
+			} else if (fixup_mcexception(regs)) {

arch/x86/kernel/cpu/mcheck/mce.c void do_machine_check(struct pt_regs *regs, long error_code) · line 1144

Quoted 4 lines

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

+				regs->ax = BIT(63) | m.addr;
+			} else
+				mce_panic("Failed kernel mode recovery",
+					  &m, NULL);

Maybe I'm misunderstanding this, but presumably you shouldn't call fixup_mcexception unless you've first verified RIPV (i.e. that the ip you're looking up in the table is valid).

Also... I find the general flow of this code very hard to follow. It's critical that an MCE hitting kernel mode not get as far as ist_begin_non_atomic. It was already hard enough to tell that the code follows that rule, and now it's even harder. Would it make sense to add clear assertions that m.cs == regs->cs and that user_mode(regs) when you get to the end? Simplifying the control flow might also be nice.

arch/x86/kernel/cpu/mcheck/mce.c void do_machine_check(struct pt_regs *regs, long error_code) · line 1147

Quoted 4 lines

arch/x86/kernel/cpu/mcheck/mce.c +0 -0

 		} else if (kill_it) {
 			force_sig(SIGBUS, current);
 		}

I would argue that this should happen in the non-atomic section. It's probably okay as long as we came from user mode, but it's more obviously safe in the non-atomic section.

Using __copy_user_nocache() as inspiration create a memory copy routine for use by kernel code with annotations to allow for recovery from machine checks.

Notes:
1) Unlike the original we make no attempt to copy all the bytes up to the faulting address. The original achieves that by re-executing the failing part as a byte-by-byte copy, which will take another page fault. We don't want to have a second machine check!
2) Likewise the return value for the original indicates exactly how many bytes were not copied. Instead we provide the physical address of the fault (thanks to help from do_machine_check()

arch/x86/include/asm/uaccess_64.h __copy_to_user_inatomic(void __user *dst, const void *src, unsigned size) · line 220

Quoted 2 lines

arch/x86/include/asm/uaccess_64.h +2 -0

+extern phys_addr_t mcsafe_memcpy(void *dst, const void __user *src,
+				unsigned size);

So what's the longer term purpose, where will mcsafe_memcpy() be used?

Quoted 4 lines · Ingo Molnar

arch/x86/include/asm/uaccess_64.h __copy_to_user_inatomic(void __user *dst, const void *src, unsigned size) · line 220

Quoted 2 lines

arch/x86/include/asm/uaccess_64.h +2 -0

+extern phys_addr_t mcsafe_memcpy(void *dst, const void __user *src,
+				unsigned size);

So what's the longer term purpose, where will mcsafe_memcpy() be used?

The initial plan is to use this for file systems backed by NVDIMMs. They will have a large amount of memory, and we have a practical recovery path - return -EIO just like legacy h/w.

We can look for other places in the kernel where we read large amounts of memory and have some idea how to recover if the memory turns out to be bad.

Quoted 10 lines · Luck, Tony

Quoted 4 lines · Ingo Molnar

arch/x86/include/asm/uaccess_64.h __copy_to_user_inatomic(void __user *dst, const void *src, unsigned size) · line 220

Quoted 2 lines

arch/x86/include/asm/uaccess_64.h +2 -0

+extern phys_addr_t mcsafe_memcpy(void *dst, const void __user *src,
+                         unsigned size);

So what's the longer term purpose, where will mcsafe_memcpy() be used?

The initial plan is to use this for file systems backed by NVDIMMs. They will have a large amount of memory, and we have a practical recovery path - return -EIO just like legacy h/w.

Quoted 2 lines

We can look for other places in the kernel where we read large amounts of memory and have some idea how to recover if the memory turns out to be bad.

I see, that's sensible!

Thanks,

Ingo

Is that an "Acked-by"? I'd like to pull this plus Vishal's gendisk-badblocks patches into a unified libnvdimm-error-handling branch. We're looking to have v4.5 able to avoid or survive nvdimm media errors through the pmem driver and DAX paths.

[ adding nvdimm folks ]

Quoted 18 lines · Ingo Molnar

Quoted 4 lines · Luck, Tony

Quoted 3 lines · Ingo Molnar

Quoted 2 lines

arch/x86/include/asm/uaccess_64.h +2 -0

+extern phys_addr_t mcsafe_memcpy(void *dst, const void __user *src,
+                         unsigned size);

Quoted 8 lines

Quoted 1 line

So what's the longer term purpose, where will mcsafe_memcpy() be used?

The initial plan is to use this for file systems backed by NVDIMMs. They will have a large amount of memory, and we have a practical recovery path - return -EIO just like legacy h/w.

We can look for other places in the kernel where we read large amounts of memory and have some idea how to recover if the memory turns out to be bad.

I see, that's sensible!

Quoted 3 lines

Thanks,

Ingo

Is that an "Acked-by"? I'd like to pull this plus Vishal's gendisk-badblocks patches into a unified libnvdimm-error-handling branch. We're looking to have v4.5 able to avoid or survive nvdimm media errors through the pmem driver and DAX paths.

This is a first draft to show the direction I'm taking to make it possible for the kernel to recover from machine checks taken while kernel code is executing.

This is a first draft to show the direction I'm taking to make it possible for the kernel to recover from machine checks taken while kernel code is executing.

Note that I also fudge the return value. I'd like in the future to be able to write a "mcsafe_copy_from_user()" function that would be annotated both for page faults, to return a count of bytes uncopied, or an indication that there was a machine check. Hence the BIT(63) bit. Internal feedback suggested we'd need some IS_ERR() like macros to help users decode what happened to take the right action. But this is "RFC" to see if people have better ideas on how to handle this.

Just a general, why-do-we-do-this, question: on big systems, the memory occupied by the kernel is a very small percentage compared to whole RAM, right? And yet we want to recover from there too? Not, say, kexec...

Quoted 8 lines

Note that I also fudge the return value. I'd like in the future to be able to write a "mcsafe_copy_from_user()" function that would be annotated both for page faults, to return a count of bytes uncopied, or an indication that there was a machine check. Hence the BIT(63) bit. Internal feedback suggested we'd need some IS_ERR() like macros to help users decode what happened to take the right action. But this is "RFC" to see if people have better ideas on how to handle this.

Hmm, shouldn't this be using MF_ACTION_REQUIRED or even maybe a new MF_ flag which is converted into a BUS_MCEERR_AR si_code and thus current gets a signal?

Only setting bit 63 looks a bit flaky to me...

I need to add more to the motivation part of this. The people who want this are playing with NVDIMMs as storage. So think of many GBytes of non-volatile memory on the source end of the memcpy(). People are used to disk errors just giving them a -EIO error. They'll be unhappy if an NVDIMM error crashes the machine.

It will be up to the caller to figure out what action to take. In the NVDIMM filessytem scenario outlined above the result may be -EIO for a data block ... something more drastic if we were reading metadata.

When I get around to writing mcsafe_copy_from_user() the code might end up like:

some_syscall_e_g_write(void __user *buf, size_t cnt)

{
	u64 ret;

	ret = mcsafe_copy_from_user(kbuf, buf, cnt);

	if (ret & BIT(63)) {
		do some machine check thing ... e.g.
		send a SIGBUS to this process and return -EINTR
		This is where we use the address (after converting
		back to a user virtual address).
	} else if (ret) {
		user gave us a bad buffer: return -EFAULT
	} else {
		success!!!
	}
}

Quoted 5 lines · Tony

I need to add more to the motivation part of this. The people who want this are playing with NVDIMMs as storage. So think of many GBytes of non-volatile memory on the source end of the memcpy(). People are used to disk errors just giving them a -EIO error. They'll be unhappy if an NVDIMM error crashes the machine.

Ah.

Btw, there's no flag, by chance, somewhere in the MCA regs bunch at error time which says that the error is originating from NVDIMM? Because if there were, this patchset is moot. :)

No flag. We can search MCi_ADDR across the ranges to see whether this was a normal RAM error on non-volatile. But that doesn't make this patch moot. We still need to change the return address to go to the fixup code instead of back to the place where we hit the error.

If you know that it is in the nvdimm range, you can grade the error with lower severity...

Or do you mean that without the exception table we'll return back to the insn causing the error and loop indefinitely this way?