Machine check recovery when kernel accesses poison

[Resend of v11 with Boris' "Reviewed-by" tags added. For Ingo's workflow]

-Tony

Tony Luck (4):
x86: Expand exception table to allow new handling options x86, mce: Check for faults tagged in EXTABLE_CLASS_FAULT exception
table entries
x86, mce: Add __mcsafe_copy()
x86: Create a new synthetic cpu capability for machine check recovery

 Documentation/x86/exception-tables.txt    |  35 +++++++
 Documentation/x86/x86_64/boot-options.txt |   2 +
 arch/x86/include/asm/asm.h                |  40 ++++----
 arch/x86/include/asm/cpufeature.h         |   1 +
 arch/x86/include/asm/mce.h                |   1 +
 arch/x86/include/asm/string_64.h          |   8 ++
 arch/x86/include/asm/uaccess.h            |  16 ++--
 arch/x86/kernel/cpu/mcheck/mce-severity.c |  22 ++++-
 arch/x86/kernel/cpu/mcheck/mce.c          |  83 +++++++++-------
 arch/x86/kernel/kprobes/core.c            |   2 +-
 arch/x86/kernel/traps.c                   |   6 +-
 arch/x86/kernel/x8664_ksyms_64.c          |   2 +
 arch/x86/lib/memcpy_64.S                  | 151 ++++++++++++++++++++++++++++++
 arch/x86/mm/extable.c                     | 100 ++++++++++++++------
 arch/x86/mm/fault.c                       |   2 +-
 scripts/sortextable.c                     |  32 +++++++
 16 files changed, 410 insertions(+), 93 deletions(-)

2.5.0

Huge amounts of help from Andy Lutomirski and Borislav Petkov to produce this. Andy provided the inspiration to add classes to the exception table with a clever bit-squeezing trick, Boris pointed out how much cleaner it would all be if we just had a new field.

Linus Torvalds blessed the expansion with:
I'd rather not be clever in order to save just a tiny amount of space in the exception table, which isn't really criticial for anybody.

The third field is another relative function pointer, this one to a handler that executes the actions.

We start out with three handlers:

1: Legacy - just jumps the to fixup IP
2: Fault - provide the trap number in %ax to the fixup code 3: Cleaned up legacy for the uaccess error hack

---
 Documentation/x86/exception-tables.txt |  35 ++++++++++++
 arch/x86/include/asm/asm.h             |  40 +++++++------
 arch/x86/include/asm/uaccess.h         |  16 +++---
 arch/x86/kernel/kprobes/core.c         |   2 +-
 arch/x86/kernel/traps.c                |   6 +-
 arch/x86/mm/extable.c                  | 100 ++++++++++++++++++++++++---------
 arch/x86/mm/fault.c                    |   2 +-
 scripts/sortextable.c                  |  32 +++++++++++
 8 files changed, 176 insertions(+), 57 deletions(-)

index 32901aa36f0a..fed18187a8b8 100644
@@ -290,3 +290,38 @@ Due to the way that the exception table is built and needs to be ordered,
 only use exceptions for code in the .text section.  Any other section
 will cause the exception table to not be sorted correctly, and the
 exceptions will fail.
+
+Things changed when 64-bit support was added to x86 Linux. Rather than
+double the size of the exception table by expanding the two entries
+from 32-bits to 64 bits, a clever trick was used to store addresses
+as relative offsets from the table itself. The assembly code changed
+from:
+	.long 1b,3b
+to:
+        .long (from) - .
+        .long (to) - .
+
+and the C-code that uses these values converts back to absolute addresses
+like this:
+
+	ex_insn_addr(const struct exception_table_entry *x)
+	{
+		return (unsigned long)&x->insn + x->insn;
+	}
+
+In v4.5 the exception table entry was given a new field "handler".
+This is also 32-bits wide and contains a third relative function
+pointer which points to one of:
+
+1) int ex_handler_default(const struct exception_table_entry *fixup)
+   This is legacy case that just jumps to the fixup code
+2) int ex_handler_fault(const struct exception_table_entry *fixup)
+   This case provides the fault number of the trap that occurred at
+   entry->insn. It is used to distinguish page faults from machine
+   check.
+3) int ex_handler_ext(const struct exception_table_entry *fixup)
+   This case is used for uaccess_err ... we need to set a flag
+   in the task structure. Before the handler functions existed this
+   case was handled by adding a large offset to the fixup to tag
+   it as special.
+More functions can easily be added.

index 189679aba703..f5063b6659eb 100644
@@ -44,19 +44,22 @@
 
 /* Exception table entry */
 #ifdef __ASSEMBLY__
-# define _ASM_EXTABLE(from,to)					\
+# define _ASM_EXTABLE_HANDLE(from, to, handler)			\
 	.pushsection "__ex_table","a" ;				\
-	.balign 8 ;						\
+	.balign 4 ;						\
 	.long (from) - . ;					\
 	.long (to) - . ;					\
+	.long (handler) - . ;					\
 	.popsection
 
-# define _ASM_EXTABLE_EX(from,to)				\
-	.pushsection "__ex_table","a" ;				\
-	.balign 8 ;						\
-	.long (from) - . ;					\
-	.long (to) - . + 0x7ffffff0 ;				\
-	.popsection
+# define _ASM_EXTABLE(from, to)					\
+	_ASM_EXTABLE_HANDLE(from, to, ex_handler_default)
+
+# define _ASM_EXTABLE_FAULT(from, to)				\
+	_ASM_EXTABLE_HANDLE(from, to, ex_handler_fault)
+
+# define _ASM_EXTABLE_EX(from, to)				\
+	_ASM_EXTABLE_HANDLE(from, to, ex_handler_ext)
 
 # define _ASM_NOKPROBE(entry)					\
 	.pushsection "_kprobe_blacklist","aw" ;			\

@@ -89,19 +92,24 @@
 	.endm
 
 #else
-# define _ASM_EXTABLE(from,to)					\
+# define _EXPAND_EXTABLE_HANDLE(x) #x
+# define _ASM_EXTABLE_HANDLE(from, to, handler)			\
 	" .pushsection \"__ex_table\",\"a\"\n"			\
-	" .balign 8\n"						\
+	" .balign 4\n"						\
 	" .long (" #from ") - .\n"				\
 	" .long (" #to ") - .\n"				\
+	" .long (" _EXPAND_EXTABLE_HANDLE(handler) ") - .\n"	\
 	" .popsection\n"
 
-# define _ASM_EXTABLE_EX(from,to)				\
-	" .pushsection \"__ex_table\",\"a\"\n"			\
-	" .balign 8\n"						\
-	" .long (" #from ") - .\n"				\
-	" .long (" #to ") - . + 0x7ffffff0\n"			\
-	" .popsection\n"
+# define _ASM_EXTABLE(from, to)					\
+	_ASM_EXTABLE_HANDLE(from, to, ex_handler_default)
+
+# define _ASM_EXTABLE_FAULT(from, to)				\
+	_ASM_EXTABLE_HANDLE(from, to, ex_handler_fault)
+
+# define _ASM_EXTABLE_EX(from, to)				\
+	_ASM_EXTABLE_HANDLE(from, to, ex_handler_ext)
+
 /* For C file, we already have NOKPROBE_SYMBOL macro */
 #endif
 

index a4a30e4b2d34..c0f27d7ea7ff 100644
@@ -90,12 +90,11 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un
 	likely(!__range_not_ok(addr, size, user_addr_max()))
 
 /*
- * The exception table consists of pairs of addresses relative to the
- * exception table enty itself: the first is the address of an
- * instruction that is allowed to fault, and the second is the address
- * at which the program should continue.  No registers are modified,
- * so it is entirely up to the continuation code to figure out what to
- * do.
+ * The exception table consists of triples of addresses relative to the
+ * exception table entry itself. The first address is of an instruction
+ * that is allowed to fault, the second is the target at which the program
+ * should continue. The third is a handler function to deal with the fault
+ * caused by the instruction in the first field.
  *
  * All the routines below use bits of fixup code that are out of line
  * with the main instruction path.  This means when everything is well,

@@ -104,13 +103,14 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un
  */
 
 struct exception_table_entry {
-	int insn, fixup;
+	int insn, fixup, handler;
 };
 /* This is not the generic standard exception_table_entry format */
 #define ARCH_HAS_SORT_EXTABLE
 #define ARCH_HAS_SEARCH_EXTABLE
 
-extern int fixup_exception(struct pt_regs *regs);
+extern int fixup_exception(struct pt_regs *regs, int trapnr);
+extern bool ex_has_fault_handler(unsigned long ip);
 extern int early_fixup_exception(unsigned long *ip);
 
 /*

index 1deffe6cc873..0f05deeff5ce 100644
@@ -988,7 +988,7 @@ int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
 		 * In case the user-specified fault handler returned
 		 * zero, try to fix up.
 		 */
-		if (fixup_exception(regs))
+		if (fixup_exception(regs, trapnr))
 			return 1;
 
 		/*

index ade185a46b1d..211c11c7bba4 100644
@@ -199,7 +199,7 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 	}
 
 	if (!user_mode(regs)) {
-		if (!fixup_exception(regs)) {
+		if (!fixup_exception(regs, trapnr)) {
 			tsk->thread.error_code = error_code;
 			tsk->thread.trap_nr = trapnr;
 			die(str, regs, error_code);

@@ -453,7 +453,7 @@ do_general_protection(struct pt_regs *regs, long error_code)
 
 	tsk = current;
 	if (!user_mode(regs)) {
-		if (fixup_exception(regs))
+		if (fixup_exception(regs, X86_TRAP_GP))
 			return;
 
 		tsk->thread.error_code = error_code;

@@ -699,7 +699,7 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 	conditional_sti(regs);
 
 	if (!user_mode(regs)) {
-		if (!fixup_exception(regs)) {
+		if (!fixup_exception(regs, trapnr)) {
 			task->thread.error_code = error_code;
 			task->thread.trap_nr = trapnr;
 			die(str, regs, error_code);

index eef44d9a3f77..495946c3f9dd 100644
@@ -656,7 +656,7 @@ no_context(struct pt_regs *regs, unsigned long error_code,
 	int sig;
 
 	/* Are we prepared to handle this kernel fault? */
-	if (fixup_exception(regs)) {
+	if (fixup_exception(regs, X86_TRAP_PF)) {
 		/*
 		 * Any interrupt that takes a fault gets the fixup. This makes
 		 * the below recursive fault logic only apply to a faults from

index c2423d913b46..7b29fb14f870 100644
@@ -209,6 +209,35 @@ static int compare_relative_table(const void *a, const void *b)
 	return 0;
 }
 
+static void x86_sort_relative_table(char *extab_image, int image_size)
+{
+	int i;
+
+	i = 0;
+	while (i < image_size) {
+		uint32_t *loc = (uint32_t *)(extab_image + i);
+
+		w(r(loc) + i, loc);
+		w(r(loc + 1) + i + 4, loc + 1);
+		w(r(loc + 2) + i + 8, loc + 2);
+
+		i += sizeof(uint32_t) * 3;
+	}
+
+	qsort(extab_image, image_size / 12, 12, compare_relative_table);
+
+	i = 0;
+	while (i < image_size) {
+		uint32_t *loc = (uint32_t *)(extab_image + i);
+
+		w(r(loc) - i, loc);
+		w(r(loc + 1) - (i + 4), loc + 1);
+		w(r(loc + 2) - (i + 8), loc + 2);
+
+		i += sizeof(uint32_t) * 3;
+	}
+}
+
 static void sort_relative_table(char *extab_image, int image_size)
 {
 	int i;

@@ -281,6 +310,9 @@ do_file(char const *const fname)
 		break;
 	case EM_386:
 	case EM_X86_64:
+		custom_sort = x86_sort_relative_table;
+		break;
+
 	case EM_S390:
 		custom_sort = sort_relative_table;
 		break;

2.5.0

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 tagged with _ASM_EXTABLE_FAULT() so that the ex_handler_fault() handler will provide the fixup code with the trap number.

Major re-work to the tail code in do_machine_check() to make all this readable/maintainable. One functional change is that tolerant=3 no longer stops recovery actions. Revert to only skipping sending SIGBUS to the current process.

---
 arch/x86/kernel/cpu/mcheck/mce-severity.c | 22 +++++++++-
 arch/x86/kernel/cpu/mcheck/mce.c          | 70 ++++++++++++++++---------------
 2 files changed, 56 insertions(+), 36 deletions(-)

index 9c682c222071..5119766d9889 100644
@@ -14,6 +14,7 @@
 #include <linux/init.h>
 #include <linux/debugfs.h>
 #include <asm/mce.h>
+#include <asm/uaccess.h>
 
 #include "mce-internal.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 in 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

@@ -170,6 +181,9 @@ static struct severity {
 		)	/* always matches. keep at end */
 };
 
+#define mc_recoverable(mcg) (((mcg) & (MCG_STATUS_RIPV|MCG_STATUS_EIPV)) == \
+				(MCG_STATUS_RIPV|MCG_STATUS_EIPV))
+
 /*
  * If mcgstatus indicated that ip/cs on the stack were
  * no good, then "m->cs" will be zero and we will have

@@ -183,7 +197,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 (mc_recoverable(m->mcgstatus) && ex_has_fault_handler(m->ip))
+		return IN_KERNEL_RECOV;
+	return IN_KERNEL;
 }
 
 /*

2.5.0

Make use of the EXTABLE_FAULT exception table entries. This routine returns a structure to indicate the result of the copy:

struct mcsafe_ret {
        u64 trapnr;
        u64 remain;
};

If the copy is successful, then both 'trapnr' and 'remain' are zero.

If we faulted during the copy, then 'trapnr' will say which type of trap (X86_TRAP_PF or X86_TRAP_MC) and 'remain' says how many bytes were not copied.

Note that this is probably the first of several copy functions. We can make new ones for non-temporal cache handling etc.

---
 arch/x86/include/asm/string_64.h |   8 +++
 arch/x86/kernel/x8664_ksyms_64.c |   2 +
 arch/x86/lib/memcpy_64.S         | 151 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 161 insertions(+)

index ff8b9a17dc4b..5b24039463a4 100644
@@ -78,6 +78,14 @@ int strcmp(const char *cs, const char *ct);
 #define memset(s, c, n) __memset(s, c, n)
 #endif
 
+struct mcsafe_ret {
+	u64 trapnr;
+	u64 remain;
+};
+
+struct mcsafe_ret __mcsafe_copy(void *dst, const void __user *src, size_t cnt);
+extern void __mcsafe_copy_end(void);
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_STRING_64_H */

index a0695be19864..fff245462a8c 100644
@@ -37,6 +37,8 @@ EXPORT_SYMBOL(__copy_user_nocache);
 EXPORT_SYMBOL(_copy_from_user);
 EXPORT_SYMBOL(_copy_to_user);
 
+EXPORT_SYMBOL_GPL(__mcsafe_copy);
+
 EXPORT_SYMBOL(copy_page);
 EXPORT_SYMBOL(clear_page);
 

2.5.0

So the series looks good to me, but I have some (mostly readability) comments that went beyond what I usually fix up manually:

  struct mcsafe_ret {
          u64 trap_nr;
          u64 bytes_left;
  };

I prefer to use my modern console width to display multiple columns of text, instead of wasting it to display mostly whitespace. Therefore I still very much prefer ~80 char wide code.

I think the whole notion of mcsafe here is 'wrong'. This copy variant simply reports the kind of trap that happened (#PF or #MC) and could arguably be extended to include more types if the hardware were to generate more.

This naming won't hurt the col80 limit.

What would a better name be? memcpy_ret() or so?

Thanks,

Ingo

It is safe in the sense that when you get an MCE while shuffling data here, in the kernel, you don't die but you recover. Thus the exception handling games.

So _safe() really sounds fitting here.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Yeah, uhmm.. naming. More options from the lack of inspiration department:

memcpy_trap()
memcpy_ex()

I like "memcpy_trap" - thanks Peter. I'll redo this part with all the other fixes suggested by Ingo.

-Tony

Btw., the main reason I hate the col80 limit is that I see such patches frequently:

void pcibios_add_bus(struct pci_bus *bus)
{
+#ifdef CONFIG_DMI

+       const struct dmi_device *dmi;
+       struct dmi_dev_onboard *dslot;
+       char sname[128];

+
+ dmi = NULL;
+ while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT,
+ NULL, dmi)) != NULL) {
+ dslot = dmi->device_data;
+ if (dslot->segment == pci_domain_nr(bus) &&
+ dslot->bus == bus->number) {
+ dev_info(&bus->dev, "Found SMBIOS Slot %s\n",
+ dslot->dev.name);
+ snprintf(sname, sizeof(sname), "%s-%d",
+ dslot->dev.name,
+ dslot->instance);
+ pci_create_slot(bus, dslot->devfn,
+ sname, NULL);
+ }
+ }
+#endif

        acpi_pci_add_bus(bus);

Which gobbledygook has 6 (!) col80 artifacts - and it's a pretty straightforward piece of code with just 2 levels of indentation.

It is IMHO much more readable in the following form:

void pcibios_add_bus(struct pci_bus *bus)
{

 #ifdef CONFIG_DMI
        const struct dmi_device *dmi;
        struct dmi_dev_onboard *dslot;
        char sname[128];

        dmi = NULL;
        while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT, NULL, dmi)) != NULL) {
                dslot = dmi->device_data;
                if (dslot->segment == pci_domain_nr(bus) && dslot->bus == bus->number) {
                        dev_info(&bus->dev, "Found SMBIOS Slot %s\n", dslot->dev.name);
                        snprintf(sname, sizeof(sname), "%s-%d", dslot->dev.name, dslot->instance);
                        pci_create_slot(bus, dslot->devfn, sname, NULL);
                }
        }
 #endif
        acpi_pci_add_bus(bus);

BYMMV.

Thanks,

Ingo

So I mostly agree, although your example does wrap on my normal display width. The thing is though, we have to have a limit, otherwise people will completely let loose and we'll end up with lines >200 chars wide (I've worked on code like that in the past, and its a right pain).

And 80 has so far mostly worked just fine. Its just that people seem unable to take guidelines as just than, and instead produce the most horrible code just to adhere to checkpatch or whatnot.

And I'd much rather have an extra column of code than waste a lot of screen-estate to display mostly whitespace.

Also, this 'artificial' limit on indentation level does in general encourage people to write saner code.

What I'm arguing for is to be, on average, _stricter_ than col80, but not use the absolute width as a metric.

Obviusly we have to have limits (to have a consistent coding style) - but I think it should be the level of indentation/nesting that should be the limit and the number of arguments to a function, while the absolute character count should be relaxed in certain cases (and should be made more strict in others!), such as printks and other 'leaf' functionality that has no primary side effects.

I'd be fine with only allowing up to 2-3 levels of nesting in typical code situations, and not having silly long names. I'd also maximize function arguments at about ~4 parameters for the typical case - anything longer should probably organize the parameters into helper structures.

But yeah, I can see the pragmatic power of a 'simple' guideline, such as col80.

I think it should be made _stricter_ in many cases.

I.e. col80 is too easy to work around and is routinely worked around in 80% of the patches I get.

So I think that with my proposed rule we'd mostly have much shorter code than 80 colums, with a few cases (such as printk() lines) where _you_ could easily accept automatic, editor generated line wraps instead of forcing the ugliness of manual line breaks on everyone else...

The fact is that in almost every patch I receieve these days I see col80 artifacts that could be avoided with better code structure - or that would look better if they were not manually line-broken.

I.e. I don't so much argue in favor of making lines longer than 80 cols, I argue against 'col80 line breaks' that are an easy workaround around the col80 rule.

But that's not what we have - col80 is in fact too permissive when it comes to actual code complexity!

Let me give a random example - took me 20 seconds to find in kernel/*.c:
kernel/cgroup.c's cgroup_subtree_control_write():

- the function is way too big with 230+ lines - it should be split into 2-4 helper functions.

- the deepest C indentation it has is too much: 4 levels

- the function has 11+ 'col80 artifacts' that I counted

- the function has similar looking code patterns repeated over it

- the control flow is messy at places - goto patterns mixed with open coded unlock sequences.

- some logic is completely undocumented - for example can you tell at a glance what the first for_each_subsys() loop does? If it was in a helper function it would be self-documenting to a large degree.

and it's a piece of code that is completely col80 compliant while it has obvious problems.

Most of the problems in this function would go away with two relatively simple rules, which are in fact stricter than col80 limits:

- not going over 3 levels deep of nesting.

- not allowing 'manual line breaks' for non-trivial functionality. This rule would flag ugly pieces of code like:

                cgroup_for_each_live_child(child, cgrp) {
                        if (css_enable & (1 << ssid))
                                ret = create_css(child, ss,
                                        cgrp->subtree_control & (1 << ssid));
                        else
                                ret = css_populate_dir(cgroup_css(child, ss),
                                                       NULL);
                        if (ret)
                                goto err_undo_css;
                }

these two rules would IMHO automatically limit the complexity of many functions that are too complex today.

Thanks,

Ingo

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

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Does the build fail - or is it just an unused function? If the latter then I'd rather leave the #ifdef out.

Thanks,

Ingo

Yep, it does fail:

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

due to ex_handler_fault not being visible to UML.

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Ok, fair enough.

Thanks,

Ingo

Make use of the EXTABLE_FAULT exception table entries. This routine returns a structure to indicate the result of the copy:

struct mcsafe_ret {
        u64 trap_nr;
        u64 bytes_left;
};

If the copy is successful, then both 'trap_nr' and 'bytes_left' are zero.

If we faulted during the copy, then 'trap_nr' will say which type of trap (X86_TRAP_PF or X86_TRAP_MC) and 'bytes_left' says how many bytes were not copied.

Note that this is probably the first of several copy functions. We can make new ones for non-temporal cache handling etc.

---

V12 (part3 only - parts 1,2,4 are in tip tree)

Ingo: More meaningful names for fields of return structure
PeterZ: Better name for copy function: memcpy_trap()
Ingo: Don't use numeric labels in new asm code
Ingo: Consistent capitalization in comments.
Ingo: Periods between sentences in comments.

Not addressed: Linus' comment that perhaps we could try/catch syntactic sugar instead of returning multiple values in a structure.

 arch/x86/include/asm/string_64.h |  26 +++++++
 arch/x86/kernel/x8664_ksyms_64.c |   2 +
 arch/x86/lib/memcpy_64.S         | 158 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 186 insertions(+)

index ff8b9a17dc4b..65e5793b7590 100644
@@ -78,6 +78,32 @@ int strcmp(const char *cs, const char *ct);
 #define memset(s, c, n) __memset(s, c, n)
 #endif
 
+/**
+ * struct memcpy_trap_ret - return value from memcpy_trap()
+ *
+ * @trap_nr	x86 trap number if the copy failed
+ * @bytes_left	zero for successful copy else number of bytes not copied
+ */
+struct memcpy_trap_ret {
+	u64 trap_nr;
+	u64 bytes_left;
+};
+
+/**
+ * memcpy_trap - copy memory with indication if a trap interrupted the copy
+ *
+ * @dst:	destination address
+ * @src:	source address
+ * @cnt:	number of bytes to copy
+ *
+ * Low level memory copy function that catches traps and indicates whether
+ * the copy succeeded and if not, why it failed.
+ *
+ * Return is struct memcpy_trap_ret which provides both the number of bytes
+ * not copied and the reason for the failure.
+ */
+struct memcpy_trap_ret memcpy_trap(void *dst, const void __user *src, size_t cnt);
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_STRING_64_H */

index a0695be19864..40866e2cbcc4 100644
@@ -37,6 +37,8 @@ EXPORT_SYMBOL(__copy_user_nocache);
 EXPORT_SYMBOL(_copy_from_user);
 EXPORT_SYMBOL(_copy_to_user);
 
+EXPORT_SYMBOL_GPL(memcpy_trap);
+
 EXPORT_SYMBOL(copy_page);
 EXPORT_SYMBOL(clear_page);
 

2.5.0

Make use of the EXTABLE_FAULT exception table entries. This routine returns a structure to indicate the result of the copy:

struct mcsafe_ret {
        u64 trap_nr;
        u64 bytes_left;
};

If the copy is successful, then both 'trap_nr' and 'bytes_left' are zero.

If we faulted during the copy, then 'trap_nr' will say which type of trap (X86_TRAP_PF or X86_TRAP_MC) and 'bytes_left' says how many bytes were not copied.

Note that this is probably the first of several copy functions. We can make new ones for non-temporal cache handling etc.

---
V12-V13
Ingo: Separate instruction arguments with a ", "
Note that I didn't add spaces after "," within an argument. E.g. "lea (%rdx,%rcx,8), %rdx"
Did you want them there too? I don't think they help as much there.
Ingo: More readable layout for fixup stubs

 arch/x86/include/asm/string_64.h |  26 ++++++++
 arch/x86/kernel/x8664_ksyms_64.c |   2 +
 arch/x86/lib/memcpy_64.S         | 128 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 156 insertions(+)

index ff8b9a17dc4b..65e5793b7590 100644
@@ -78,6 +78,32 @@ int strcmp(const char *cs, const char *ct);
 #define memset(s, c, n) __memset(s, c, n)
 #endif
 
+/**
+ * struct memcpy_trap_ret - return value from memcpy_trap()
+ *
+ * @trap_nr	x86 trap number if the copy failed
+ * @bytes_left	zero for successful copy else number of bytes not copied
+ */
+struct memcpy_trap_ret {
+	u64 trap_nr;
+	u64 bytes_left;
+};
+
+/**
+ * memcpy_trap - copy memory with indication if a trap interrupted the copy
+ *
+ * @dst:	destination address
+ * @src:	source address
+ * @cnt:	number of bytes to copy
+ *
+ * Low level memory copy function that catches traps and indicates whether
+ * the copy succeeded and if not, why it failed.
+ *
+ * Return is struct memcpy_trap_ret which provides both the number of bytes
+ * not copied and the reason for the failure.
+ */
+struct memcpy_trap_ret memcpy_trap(void *dst, const void __user *src, size_t cnt);
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_STRING_64_H */

index a0695be19864..40866e2cbcc4 100644
@@ -37,6 +37,8 @@ EXPORT_SYMBOL(__copy_user_nocache);
 EXPORT_SYMBOL(_copy_from_user);
 EXPORT_SYMBOL(_copy_to_user);
 
+EXPORT_SYMBOL_GPL(memcpy_trap);
+
 EXPORT_SYMBOL(copy_page);
 EXPORT_SYMBOL(clear_page);
 

2.5.0

Where do we stand with this? The followup discussion dropped LKML at some point in the thread ... so here is the summary to bring the archive up to date:

1) Dan Williams doesn't really care about getting the bytes_left value. A simple succeed/fail code would work for him.

2) But if we want to use this for copy_from_user() as part of the write(2) call stack (and I *do* want to do that), then there are some POSIX corner cases that say that if the middle of a buffer supplied by the user is invalid we should write bytes up to that point to the file and return a short, but accurate, byte count rather than -EFAULT

3) Linus was concerned that we would not be able to get a precise bytes_left value when using the "rep mov" x86ism because it might be copying in a weird order (even backwards) for speed reasons. But the Intel architects pointed to the SDM volume 2 "REP" description which makes it clear that whatever shenanigans might be happening behind the scenes, if the "rep" is interrupted by a trap or fault the architectural view will be that all bytes up to the point of the fault will have been copied, no bytes beyond that point will have been copied (in flight writes will be dropped). rdi/rsi/ecx registers will all have been updated to the point of the fault (so we somehow fixed the reason for the machine check we'l be able to *continue* the copy from the point where it faulted).

-Tony

I don't think that is even remotely an option.

If doing a rep movs can cause the chip to crash, we cannot possibly allow user space to map these pages at all. User space could just do "rep movs" on its own, intentionally or by mistake.

Put another way: if we cannot use the regular copy_from_user(), then the whole concept is already dead, dead, dead.

I would suggest you instead just make regular "copy_from_user()" work with MCE's (which may well mean "working with hardware people to make sure it isn't a machine-killing experience").

Think of it this way: if a regular copy_from_user() doesn't work on the memory, there's no way in hell we can allow user space to map it anyway.

And once a regular copy_from_user() does work on it, there is no longer any possible advantage to "memcpy_trap()".

In other words, in no situation does it make sense to make memcpy_trap() work on user addresses. It really is that simple.

Linus

I see I have caused confusion by talking about Dan's NVDIMM case and copy_from_user() in the same breath,

This isn't just about NVDIMMs. It is about uncorrected errors in any type of memory.

The copy_from_user() case I'd like to fix is when there is an uncorrected in memory. This can happen to regular DDR3/DDR4 memory just as it can happen to NVDIMM. When a user process directly reads the location with the uncorrected error the h/w triggers a machine check and if the error is marked as recoverable the kernel will SIGBUS the process. See mm/memory-failure.c

We do have an issue in current processor implementations that access using rep mov generates a fatal machine check. This is a gap and will at some point be fixed.

Currently if the user passes the address of the location with the uncorrected error to the kernel via a system call like write(2) the kernel will do the access, and we will crash. I'd like to fix that and SIGBUS the user, just like would happen if they touched the memory themselves.To do that the copy_from_user() needs to be able to tell that there was a machine check (and probably take action inline, as fixing every place that we call copy_from_user() sounds like a silly idea).

Maybe this won't be ready for inclusion in the kernel until rep mov generates recoverable machine checks ... otherwise we have a bigger heap of possible copy routines to choose from as we'd have to look not only an the speed of various copy algorithms, but also at the behavior during exceptions ... in particular users may have to choose between speed (rep mov) and recoverability on current generation cpus (and given how rare uncorrected errors are, they might all choose speed).

-Tony

But the way to fix that is absolutely *not* to introduce some new concept.

You'd just extend the _existing_ get_user() and put_user() functions to return a different error code for a memory error. Instead of -EFAULT, maybe they can return -EMEMERR or something.

I do not see how it could possibly ever make sense to introduce a new name and then make old users use that new name instead.

Linus

So if we do that we should first fix these hard coded assumptions about uaccess return codes:

triton:~/tip> git grep -E '== -EFAULT' | wc -l
44

But yes, your suggestion sounds a lot cleaner and a lot more powerful overall.

AFAICS we'll still need this exception table extension commit:

548acf19234d x86/mm: Expand the exception table logic to allow new handling options

to propagate the error code to actual uaccess methods, right?

Alternatively we could change the exception handling protocol and change _all_ uaccess methods all at once, but I think that would be crazy complex and risky. I'd rather change key uaccess methods step by step.

So if everyone agrees then I'll keep this commit queued up. The followup patches will have to be reworked according to Linus's suggestions.

Thanks,

Ingo

For reference below is what I'd hoped to be able to do with copy_from_user() [obviously needs to not just replace that ALTERNATIVE_2 setup in _copy_from_user ... would have to invent an ALTERNATIVE_3 to pick the new function for people willing to sacrifice speed for recoverability]

BUT ... there are a maze of twisty little other places that also need to be fixed:

1) There is iov_iter_fault_in_readable() that uses fault_in_pages_readable() to pre-fault the first (and last) page using a raw __get_user() call.
2) I tried to avoid that by injecting my error to the middle of a page, but still ended up in a "rep mov" copy function instead of mine. Not sure which one because no output from the core that died. :-(

-Tony

index 982ce34f4a9b..e31d8964ac09 100644
@@ -38,11 +38,15 @@ ENTRY(_copy_from_user)
 	jc bad_from_user
 	cmpq TI_addr_limit(%rax),%rcx
 	ja bad_from_user
+#if 1
+	jmp copy_from_user_with_mce_check
+#else
 	ALTERNATIVE_2 "jmp copy_user_generic_unrolled",		\
 		      "jmp copy_user_generic_string",		\
 		      X86_FEATURE_REP_GOOD,			\
 		      "jmp copy_user_enhanced_fast_string",	\
 		      X86_FEATURE_ERMS
+#endif
 ENDPROC(_copy_from_user)
 
 	.section .fixup,"ax"

index 0a42327a59d7..c377a70474e0 100644
@@ -6,6 +6,8 @@
  * Copyright 2002 Andi Kleen <[email protected]>
  */
 #include <linux/module.h>
+#include <linux/mm.h>
+#include <asm/traps.h>
 #include <asm/uaccess.h>
 
 /*

@@ -86,3 +88,25 @@ copy_user_handle_tail(char *to, char *from, unsigned len)
 		memset(to, 0, len);
 	return len;
 }
+
+__visible unsigned long
+copy_from_user_with_mce_check(void *to, const void __user *from, unsigned n)
+{
+	struct memcpy_trap_ret r;
+
+	stac();
+	r = memcpy_trap(to, (__force void *)from, n);
+	clac();
+
+	if (likely(r.bytes_left == 0))
+		return 0;
+
+	if (r.trap_nr == X86_TRAP_MC) {
+		volatile void *fault_addr = (volatile void *)from + n - r.bytes_left;
+		phys_addr_t p = virt_to_phys(fault_addr);
+
+		memory_failure(p >> PAGE_SHIFT, MCE_VECTOR, 0);
+	}
+
+	return r.bytes_left;
+}

Why?

I really don't see why you are so obsessed with replacing our current thing.

Just replace the error number in the exception path slow handling. That's *all*.

This should all be entirely about just the exception itself doing that memory failure accounting, and the actual copying code shouldn't be different or care about things AT ALL outside of the fact that we need to replace the -EFAULT with something else.

I will keep on NAK'ing these insane "let's replace the copy code" patches. I'll do it forever if that is what it takes.

And no, if the issue is that you want to replace "rep movs", then I will _still_ keep NAK'ing them.

Because if the hardware gets it wrong, then there is absolutely no point in us special-casing one of the _last_ common memory operations in the whole system.

The user copying is literally a drop in the ocean. User space does a lot more memcpy() on its own than we will *ever* copy data from user space.

Now, if the patch starts looking more like

- return -EFAULT;
+ return memory_access_fault():

where "memory_access_fault()" might do something like "current_thread_info()->trap_nr == X86_TRAP_MC ? -EBADMEM : -EFAULT", now *then* we'll be talking.

But doing things like

+ if (r.trap_nr == X86_TRAP_MC) {
+ volatile void *fault_addr = (volatile void *)from + n
- r.bytes_left;
+ phys_addr_t p = virt_to_phys(fault_addr);
+
+ memory_failure(p >> PAGE_SHIFT, MCE_VECTOR, 0);
+ }

in the copying code is insane, because dammit, that should be done by the codethat sets X86_TRAP_MC in the first place.

And if there is hardware that raises a machine check without actually telling you why - including the address - then it's laugable to talk about "recoverability" and "hardening" and things like that. Then the hardware is just broken.

Linus

Impossible as such, I think :(

do_machine_check uses IST, the memory failure code can sleep, and you can't sleep in IST context. There's a special escape that lets memory_failure sleep *if* it came from user mode.

Here's the solution I'd prefer. Change all the copy string to/from user code to use the new enhanced fixup code. Have the new fixup handler (which can be a short C function!) fix up regs->ip to point to copy_user_handle_tail and add a new parameter to copy_user_handle_tail indicating the fault type. Then put whatever fixup logic is needed in copy_user_handle_tail -- it knows the failing address (obviously), and it's running in process context with interrupts on (unless we're in a pagefault_disable section), and it can do whatever it needs to do.

Linus, it's kind of like yours, except with the trap info explicitly passed to the fixup handler instead of having the fixup handler fish it out of some per-thread structure.

Here are different some ideas I don't like.:

1. The machine check does an IPI-to-self and the failure code runs in IRQ context.

2. The machine check code rewrites the return stack to inject a function call. I don't love this.

3. Drop the idea of sending an immediate sigbus and do it with task_work. Maybe this is bad for some reason other than code messiness.

4. Change the entry code so machine check runs on the normal stack if it hits with IRQs on.

Make use of the EXTABLE_FAULT exception table entries to write a kernel copy routine that doesn't crash the system if it encounters a machine check. Prime use case for this is to copy from large arrays of non-volatile memory used as storage.

We have to use an unrolled copy loop for now because current hardware implementations treat a machine check in "rep mov" as fatal. When that is fixed we can simplify.

---

Is this what we want now? Return type is a "bool". True means that we copied OK, false means that it didn't (this is all that Dan says that he needs). Dropped all the complex code to figure out how many bytes we didn't copy as Linus says this isn't the right place to do this (and besides we should just make "rep mov" work). Changed the name from "_trap" since we no longer return the trap number. I'm not wedded to "memcpy_mcsafe" though, so feel free to suggest alternates.

Nothing to do with copy_from_user() in here ... just completing the patch series that we need for non-volatile storage.

 arch/x86/include/asm/string_64.h |  13 +++++
 arch/x86/kernel/x8664_ksyms_64.c |   2 +
 arch/x86/lib/memcpy_64.S         | 117 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+)

index ff8b9a17dc4b..ca6ba3607705 100644
@@ -78,6 +78,19 @@ int strcmp(const char *cs, const char *ct);
 #define memset(s, c, n) __memset(s, c, n)
 #endif
 
+/**
+ * memcpy_mcsafe - copy memory with indication if a machine check happened
+ *
+ * @dst:	destination address
+ * @src:	source address
+ * @cnt:	number of bytes to copy
+ *
+ * Low level memory copy function that catches machine checks
+ *
+ * Return true for success, false for fail
+ */
+bool memcpy_mcsafe(void *dst, const void *src, size_t cnt);
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_X86_STRING_64_H */

index a0695be19864..cd05942bc918 100644
@@ -37,6 +37,8 @@ EXPORT_SYMBOL(__copy_user_nocache);
 EXPORT_SYMBOL(_copy_from_user);
 EXPORT_SYMBOL(_copy_to_user);
 
+EXPORT_SYMBOL_GPL(memcpy_mcsafe);
+
 EXPORT_SYMBOL(copy_page);
 EXPORT_SYMBOL(clear_page);
 

2.5.0

Ping.

Anything more needed for this? In his last message Linus seemed OK with a *kernel* copy function that avoided death by machine check. He said:

What a "memcpy_fault()" (or whatever it would be called) means is that the kernel is doing its own copies, but knows that there is some fragility involved, and wants to have a recovery mechanism that isn't "oops, we got a machine check in the kernel, now we need to kill the machine".

The only things left to argue are the name, and the return value.

-Tony

Aaargh! -ETOOMUCHSHELLSCRIPTPROGRAMMING

-Tony

Options to fix this:
1) Just change the comments in the code.
This seems like it would confuse people as I thing most people would expect the "true" return to mean the copy succeeded.
2) Reverse the return values.
Better that option 1 - but doesn't leave scope to return a count if some future user does want to know where the copy failed.
3) Change the return type back from "bool" to "int"

     0 == success, non-zero == fail (with option to put the non-copied

byte count in later).
4) Something else

-Tony

You could return 0 for success and -EIO for failure (positive value for 'location' if that support ever resurfaces).

That would at least let me drop the ternary conversion I have in memcpy_from_pmem() [1] to convert bool to an error code.

[1]: https://patchwork.kernel.org/patch/8559521/

Please use the copy_*_user() memory copying API semantics, which are: return negative code (-EFAULT) on error, 0 on success.

Don't return 1 and please don't use bools for any memory copy library functionality.

Thanks,

Ingo

Those are the get_user/put_user() semantics.

copy_*_user() has those annoying "bytes left uncopied" return values that I really wouldn't encourage anybody else use unless they really have to. There are (good) reasons why it's done that way, but it's still not something that should be aped without the same kind of major reasons.

Linus

Indeed, and I even grepped for it because I tend to get it wrong ... I suck!

Yeah, so IIRC the main reason being security and the ability to know which bits of a possibly uninitialized area are not yet copied on.

But with primary memcpy_mcsafe() usage being in-kernel (MM)IO space management I don't think we have the 'uninitialized' problem in nearly as marked a fashion, and we very much have the (largely uncompensated) cost of more complex assembly.

Nevertheless, wrt. error handling, the bit I feel very strongly about is the following:

We have the existing 'mempcy API check for failure' pattern of:

	if (put_user()) {
		... it didn't succeed ...
	}

	if (copy_from_user()) {
		... it didn't succeed ...
	}

... which should be kept IMHO:

	if (memcpy_mcsafe()) {
		... it didn't succeed ...
	}

... and what I most disagree with most is one of the suggestions in the original mail I replied to:

    (== return 1 on success, 0 on failure)

... that's crazy IMHO and reverses the pattern of almost every memcpy-with-check (or syscall-success) API we have!

So I still think we should have the regular '0 on success, negative error value on failure' semantics - but I'd not be hugely against a '0 on success, bytes left uncopied on failure' variant either in principle, except that I think it unduly complicates the assembly side to recover the bytes.

The '0/1' current implementation is really neither of these but still fits the common error checking pattern, but it's still much better than the '1/0' suggestion which is crazy!

So I think we should move from 0/1 to 0/-EFAULT for now, and maybe (maybe) move to a 0/bytes_left return code in the future, if there comes a strong usecase. Most error checks will have to be converted in that case anyway to make use of the return code, so it's not like we introduce an extra cost here by going to 0/-EFAULT.

Also, by going to 0/-EFAULT we have the option to also allow other negative error codes, which would allow the distinction of #MC traps from regular page fault traps for example. (a driver might want to log them differently.)

Thanks,

Ingo

Returning a 'bool' was very unpopular. Doubly so because the code was just wrong (returning zero for true, one for false; great for shell programming, not so good for C).

Change return type to "int". Keep zero as the success indicator because it matches other similar code and people may be more comfortable writing:

	if (memcpy_mcsafe(to, from, count)) {
		printk("Sad panda, copy failed\n");
		...
	}

Make the failure return value -EFAULT for now.

Reported by: Mika Penttilä <[email protected]>

---
 arch/x86/include/asm/string_64.h | 4 ++--
 arch/x86/lib/memcpy_64.S         | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

index ca6ba3607705..90dbbd9666d4 100644
@@ -87,9 +87,9 @@ int strcmp(const char *cs, const char *ct);
  *
  * Low level memory copy function that catches machine checks
  *
- * Return true for success, false for fail
+ * Return 0 for success, -EFAULT for fail
  */
-bool memcpy_mcsafe(void *dst, const void *src, size_t cnt);
+int memcpy_mcsafe(void *dst, const void *src, size_t cnt);
 
 #endif /* __KERNEL__ */
 

index cbb8ee5830ff..2ec0b0abbfaa 100644
@@ -1,6 +1,7 @@
 /* Copyright 2002 Andi Kleen */
 
 #include <linux/linkage.h>
+#include <asm/errno.h>
 #include <asm/cpufeatures.h>
 #include <asm/alternative-asm.h>
 

@@ -268,16 +269,16 @@ ENTRY(memcpy_mcsafe)
 	decl %ecx
 	jnz .L_copy_trailing_bytes
 
-	/* Copy successful. Return true */
+	/* Copy successful. Return zero */
 .L_done_memcpy_trap:
 	xorq %rax, %rax
 	ret
 ENDPROC(memcpy_mcsafe)
 
 	.section .fixup, "ax"
-	/* Return false for any failure */
+	/* Return -EFAULT for any failure */
 .L_memcpy_mcsafe_fail:
-	mov	$1, %rax
+	mov	$-EFAULT, %rax
 	ret
 
 	.previous

2.5.0

So?

Just save it away in a list (we've got the NMI-safe lists and everything), and make sure to cause a work to be scheduled eventually or something. People do things in NMI's, we can do this in a machine check.

That's what we always do for any other interrupt-time thing that want to sleep, it's not even like this is unusual or special.

But at NO point does it make sense to try to handle it from "copy_from_user()". There are so many reason *not* to do it there, but the most basic one is that the error doesn't necessarily happen at copy_to_user() time in the first place. If user space has a piece of RAM that can cause machine checks, then normal user memory access may have caused the MC. And that's much _much_ more likely than "copy_from_user()", which is such a rare little special case.

copy_from_user() is simply not that special. Having it do some odd magic error handling that nobody else does is just insane.

I really think that's way too over-engineered.

We already *have* this, for special cases like user accesses. That's what our exception tables are all about.

So for our bog-standard normal copy_from_user(), we already can abort the copy in the middle. Absolutely no new infrastructure needed. No change to copy_from_user(), no nothing.

For other cases, when we don't have an exception table entry? Do the same thing the page fault code does. If it's in the kernel, we're going to be in trouble. If it's in user space, send a SIGBUS.

But again, none of that has anything to do with changing the existing copy_from_user(). The only change I see that makes sense is to have some way to change the error code.

For copy_from_user(), we get the "immediate" reaction exactly thanks to the whole exception table mechanism that that function already uses.

For everything else, it's going to have to be delayed or killed some way. And yes, the SIGBUS will be delayed until it gets back to user space.

The kernel getting a machine check while it's accessing kernel data structures? It's going to be messy. It's going to fail. Tough. That's kind of inevitable. There is no sane way to recover. The only way to recover is to have reliable hardware, or just killing the machine entirely and starting again (in a data center model). In many other situations, you're likely going to just force an oops and kill the machine. Or maybe force an oops, and decide to just hope for the best, and ignore the error entirely.

Because what choice do you have?

None of this is new.

I really think you concentrate too much on the small details. I don't see why an IST couldn't use the exception handling mechanism, for example. I don't see why an IST couldn't just add the list entry and make some later thing happen. We use ist's for debug traps already, it's not like sending a signal or looking up the exception table is fundamentally hard there, or queueing a list or whatever.

Linus

[...]

Then let's answer the API question instead of the implementation question.

If a user program accesses a bad virtual address directly, it gets SIGSEGV or SIGBUS depending on the nature of the error. This is long-established practice. The SIGSEGV case is programmer error and the SIGBUS case might be an IO error.

If a user program accesses a bad virtual address by passing the address to a syscall, it gets EFAULT. This may be programmer error or and underlying IO error, and the program can't tell.

If a user program accesses a bad address on an NVDIMM via mmap, what should happen? If mmaped NVDIMM (or other DAX space) is the same as existing poisoned memory, the program gets SIGBUS. This still makes sense.

The question here: what happens if a program accesses a bad NVDIMM address by passing a pointer to a syscall? With Tony's patches as written, I think the program gets SIGBUS via memory_failure. Do we want that behavior? If we take your suggestion and change only the error code, then the program will *not* get SIGBUS. Instead it will get -EFAULT or -ESOMETHINGELSE. Is that okay? If it is, then everything is straightforward and nothing in my previous email is relevant.

--Andy

Yes. I don't think there's much question there.

It would be a SIGBUS, and then we should fill in something sane in the siginfo. So we'd have a si_code that makes sense.

We already have BUS_MCEERR_AR and BUS_MCEERR_AO ("action required" vs "action optional"), and that together with si_addr is presumably sufficient.

So I don't think the signal delivery has any questionable issues, it's fairly obvious what the ABI already is.

Agreed.

I do think we want that behavior.

The traditional UNIX EFAULT behavior is an odd case, and the fact that we do *not* send a SIGSEGV for faults that the kernel notices is strange.

In fact, if you read POSIX, you'll notice that the standard often says that it's implementation-defined, and the reason is that a lot of system calls aren't "native" - they are wrappers in various libraries, and depending on just how things work, the actual access to a bad address may be done by the kernel (-EFAULT) or by the library wrapper (SIGSEGV).

So the EFAULT behavior is actually pretty nasty. I don't think we should necessarily use that as "this is how things should be done".

So I'd much rather say that "if you get a machine check on a user address, we'll always queue a SIGBUS". Obviously, if it happens in a system call, the system call itself will also return an error. And also obviously, there's a limit to the queueing, so if you take *multiple* machine checks in one system call, maybe you'd get just one signal.

So I'd suggest that the *signal* be sent by the machine check handler, and that it be done independently of the system call error we choose.

And in fact, if user mode is happy with that, it would certainly be easier for us to just always return -EFAULT for the error, and a user app that cares about machine checks will do all the error handling in their signal handler.

There would be advantages to that not just for kernel simplicity: if we start using a *new* error code, existing programs might be confused. But that's a pretty small advantage.

And there are certainly also advantages to coming up with a new system call error return value.

My gut feel is that people would probably prefer something other than EFAULT. Especially if we already have an error code that libraries already have a reasonable error string for. The extra complexity in the kernel to add another error case isn't _that_ big.

Of course, if users actually come and say "we don't care at all about the error code if we always get a signal, and we want the signal anyway in order to get the address that is bad", then we just shouldn't bother.

So I don't really have any very strong opinions there.

My strong opinions here really seem to be limited to "I don't think it makes sense to have a special magical copy_to/from_user() implementation".

[ Side note: *within* the confines of purely the kernel, and libnvdimm, I do think that something like "memcpy_fault()" absolutely makes sense.

So to me, "memcpy_fault()" is not a bad idea: that's a "this is
explicitly a recoverable memory copy operation within the kernel", and that's a completely separate thing from "copy_to/from_user()" that is also recoverable.

What a "memcpy_fault()" (or whatever it would be called) means is
that the kernel is doing its own copies, but knows that there is some fragility involved, and wants to have a recovery mechanism that isn't "oops, we got a machine check in the kernel, now we need to kill the machine".

That "memcpy_fault()" would just use the same exception handling
mechanism to not make it a fatal error. We already do that for possible speculative page-crossers in kernel memory for "get_unaligned_zeropad()", for example, which is a pure in-kernel fetch that we just know might fault.

So it's literally just "copy_to/from_user()" that I don't think
should have special cases outside of perhaps error returns ]

Linus

So apart from the naming, a couple of questions:

- I'd like to see the actual *use* case explained, not just what it does.

- why does this use the complex - and slower, on modern machines -
unrolled manual memory copy, when you might as well just use a single

rep ; movsb

which not only makes it smaller, but makes the exception fixup trivial.

- why not make the "bytes remaining" the same as for a user-space
copy (ie return it as the return value)?

- at that point, it ends up looking a *lot* like uaccess_try/catch,
which gets the error code from current_thread_info()->uaccess_err

Hmm?

Linus

memcpy_try()/memcpy_catch() definitely has a nice ring to it.

Thanks,

Ingo

First user is libnvdimm. Dan Williams already has code to use this so that kernel code accessing persistent memory can return -EIO to a user instead of crashing the system if the cpu runs into an uncorrected error during the copy.

I would also lkie use this for a machine check aware copy_from_user() which would avoid crashing the kernel when the uncorrected error is in a user page (we can SIGBUS the user just like we do if the user touched the poison themself).

copy_to_user() is also interesting if the source address is the page cache. I think we can also avoid crashing the kernel in this case too - but I haven't thought that all the way through.

Because current generation cpus don't give a recoverable machine check if we consume with a "rep ; movsb" :-(
When we have that we can pick the best copy function based on the capabilities of the cpu we are running on.

For my copy_from_user/copy_to_user cases we need to know both the number of remaining bytes and also *why* we stopped copying. We might have #PF, in which case we return -EFAULT to the user, if we have #MC then the recovery path is different (need to offline the page, SIGBUS the user, ...)

-Tony

Are these the memcpy_*_pmem() calls in drivers/nvdimm/pmem.c? Is there any actual patch to look at?

Thanks,

Ingo

Here's the integration patch I had from the version of mcsafe_copy() at the beginning of January. Pardon the whitespace damage... the original thread is here: [1]. Note that the "badblocks" intergration portion of that set went upstream in v4.5-rc1.

[1]: https://lists.01.org/pipermail/linux-nvdimm/2016-January/003864.html

---

Subject: x86, pmem: use __mcsafe_copy() for memcpy_from_pmem()

In support of large capacity persistent memory use __mcsafe_copy() for pmem I/O. This allows the pmem driver to support an error model similar to disks when machine check recovery is available.

---
 arch/x86/include/asm/pmem.h |   16 ++++++++++++++++
 drivers/nvdimm/Kconfig      |    1 +
 drivers/nvdimm/pmem.c       |   10 ++++++----
 include/linux/pmem.h        |   17 +++++++++++++----
 4 files changed, 36 insertions(+), 8 deletions(-)

index d8ce3ec816ab..4ef301e78a2b 100644
@@ -17,6 +17,7 @@
 #include <asm/cacheflush.h>
 #include <asm/cpufeature.h>
 #include <asm/special_insns.h>
+#include <asm/string.h>
 
 #ifdef CONFIG_ARCH_HAS_PMEM_API
 /**

@@ -47,6 +48,21 @@ static inline void arch_memcpy_to_pmem(void __pmem

*dst, const void *src,

                BUG();

}

+static inline int arch_memcpy_from_pmem(void *dst, const void __pmem *src,
+ size_t n)
+{
+ if (IS_ENABLED(CONFIG_MCE_KERNEL_RECOVERY)) {
+ struct mcsafe_ret ret;
+
+ ret = __mcsafe_copy(dst, (void __force *) src, n);
+ if (ret.remain)
+ return -EIO;
+ return 0;

+       }
+       memcpy(dst, (void __force *) src, n);
+       return 0;

+}
+
/**
* arch_wmb_pmem - synchronize writes to persistent memory *

index 53c11621d5b1..fe5885d01fd8 100644
@@ -22,6 +22,7 @@ config BLK_DEV_PMEM
        depends on HAS_IOMEM
        select ND_BTT if BTT
        select ND_PFN if NVDIMM_PFN
+       select MCE_KERNEL_RECOVERY if X86_MCE && X86_64
        help
          Memory ranges for PMEM are described by either an NFIT
          (NVDIMM Firmware Interface Table, see CONFIG_NFIT_ACPI), a

index 8744235b5be2..d8e14e962327 100644
@@ -62,6 +62,7 @@ static bool is_bad_pmem(struct badblocks *bb,

sector_t sector, unsigned int len)
static int pmem_do_bvec(struct block_device *bdev, struct page *page,
unsigned int len, unsigned int off, int rw, sector_t sector)
{

+       int rc = 0;
        void *mem = kmap_atomic(page);
        struct gendisk *disk = bdev->bd_disk;
        struct pmem_device *pmem = disk->private_data;
@@ -71,7 +72,7 @@ static int pmem_do_bvec(struct block_device *bdev,
struct page *page,
        if (rw == READ) {
                if (unlikely(is_bad_pmem(disk->bb, sector, len)))
                        return -EIO;
-               memcpy_from_pmem(mem + off, pmem_addr, len);
+               rc = memcpy_from_pmem(mem + off, pmem_addr, len);
                flush_dcache_page(page);
        } else {
                flush_dcache_page(page);
@@ -79,7 +80,7 @@ static int pmem_do_bvec(struct block_device *bdev,
struct page *page,
        }

        kunmap_atomic(mem);

- return 0;
+ return rc;
}

static blk_qc_t pmem_make_request(struct request_queue *q, struct bio *bio)
@@ -237,6 +238,7 @@ static int pmem_rw_bytes(struct nd_namespace_common *ndns,
resource_size_t offset, void *buf, size_t size, int rw)

 {
        struct pmem_device *pmem = dev_get_drvdata(ndns->claim);

+ int rc = 0;

        if (unlikely(offset + size > pmem->size)) {
                dev_WARN_ONCE(&ndns->dev, 1, "request out of range\n");
@@ -244,13 +246,13 @@ static int pmem_rw_bytes(struct nd_namespace_common *ndns,
        }

        if (rw == READ)
-               memcpy_from_pmem(buf, pmem->virt_addr + offset, size);
+               rc = memcpy_from_pmem(buf, pmem->virt_addr + offset, size);
        else {
                memcpy_to_pmem(pmem->virt_addr + offset, buf, size);
                wmb_pmem();
        }

- return 0;
+ return rc;
}

static int nd_pfn_init(struct nd_pfn *nd_pfn)

index acfea8ce4a07..0e57a5beab21 100644
@@ -42,6 +42,13 @@ static inline void arch_memcpy_to_pmem(void __pmem

*dst, const void *src,

        BUG();

}

+static inline int arch_memcpy_from_pmem(void *dst,
+ const void __pmem *src, size_t n)
+{
+ BUG();
+ return 0;
+}
+
static inline size_t arch_copy_from_iter_pmem(void __pmem *addr, size_t bytes,
struct iov_iter *i)
{
@@ -57,12 +64,14 @@ static inline void arch_clear_pmem(void __pmem *addr, size_t size)

/*
* Architectures that define ARCH_HAS_PMEM_API must provide
- * implementations for arch_memcpy_to_pmem(), arch_wmb_pmem(),
- * arch_copy_from_iter_pmem(), arch_clear_pmem() and arch_has_wmb_pmem().
+ * implementations for arch_memcpy_to_pmem(), arch_memcpy_from_pmem(),
+ * arch_wmb_pmem(), arch_copy_from_iter_pmem(), arch_clear_pmem() and
+ * arch_has_wmb_pmem().
*/
-static inline void memcpy_from_pmem(void *dst, void __pmem const *src, size_t size)
+static inline int memcpy_from_pmem(void *dst, void __pmem const *src,
+ size_t size)
{
- memcpy(dst, (void __force const *) src, size);
+ return arch_memcpy_from_pmem(dst, src, size);
}

static inline bool arch_has_pmem_api(void)

The Intel Software Developer Manual describes bit 24 in the MCG_CAP MSR:
MCG_SER_P (software error recovery support present) flag, bit 24 — Indicates (when set) that the processor supports software error recovery
But only some models with this capability bit set will actually generate recoverable machine checks.

Check the model name and set a synthetic capability bit. Provide a command line option to set this bit anyway in case the kernel doesn't recognise the model name.

---
 Documentation/x86/x86_64/boot-options.txt |  2 ++
 arch/x86/include/asm/cpufeature.h         |  1 +
 arch/x86/include/asm/mce.h                |  1 +
 arch/x86/kernel/cpu/mcheck/mce.c          | 13 +++++++++++++
 4 files changed, 17 insertions(+)

index 68ed3114c363..0965a71f9942 100644
@@ -60,6 +60,8 @@ Machine check
 		threshold to 1. Enabling this may make memory predictive failure
 		analysis less effective if the bios sets thresholds for memory
 		errors since we will not see details for all errors.
+   mce=recovery
+		Force-enable recoverable machine check code paths
 
    nomce (for compatibility with i386): same as mce=off
 

index 7ad8c9464297..06c6c2d2fea0 100644
@@ -106,6 +106,7 @@
 #define X86_FEATURE_APERFMPERF	( 3*32+28) /* APERFMPERF */
 #define X86_FEATURE_EAGER_FPU	( 3*32+29) /* "eagerfpu" Non lazy FPU restore */
 #define X86_FEATURE_NONSTOP_TSC_S3 ( 3*32+30) /* TSC doesn't stop in S3 state */
+#define X86_FEATURE_MCE_RECOVERY ( 3*32+31) /* cpu has recoverable machine checks */
 
 /* Intel-defined CPU features, CPUID level 0x00000001 (ecx), word 4 */
 #define X86_FEATURE_XMM3	( 4*32+ 0) /* "pni" SSE-3 */

index 2ea4527e462f..18d2ba9c8e44 100644
@@ -113,6 +113,7 @@ struct mca_config {
 	bool ignore_ce;
 	bool disabled;
 	bool ser;
+	bool recovery;
 	bool bios_cmci_threshold;
 	u8 banks;
 	s8 bootlog;

index 905f3070f412..15ff6f07bd92 100644
@@ -1578,6 +1578,17 @@ static int __mcheck_cpu_apply_quirks(struct cpuinfo_x86 *c)
 
 		if (c->x86 == 6 && c->x86_model == 45)
 			quirk_no_way_out = quirk_sandybridge_ifu;
+		/*
+		 * MCG_CAP.MCG_SER_P is necessary but not sufficient to know
+		 * whether this processor will actually generate recoverable
+		 * machine checks. Check to see if this is an E7 model Xeon.
+		 * We can't do a model number check because E5 and E7 use the
+		 * same model number. E5 doesn't support recovery, E7 does.
+		 */
+		if (mca_cfg.recovery || (mca_cfg.ser &&
+			!strncmp(c->x86_model_id,
+				 "Intel(R) Xeon(R) CPU E7-", 24)))
+			set_cpu_cap(c, X86_FEATURE_MCE_RECOVERY);
 	}
 	if (cfg->monarch_timeout < 0)
 		cfg->monarch_timeout = 0;

@@ -2030,6 +2041,8 @@ static int __init mcheck_enable(char *str)
 		cfg->bootlog = (str[0] == 'b');
 	else if (!strcmp(str, "bios_cmci_threshold"))
 		cfg->bios_cmci_threshold = true;
+	else if (!strcmp(str, "recovery"))
+		cfg->recovery = true;
 	else if (isdigit(str[0])) {
 		if (get_option(&str, &cfg->tolerant) == 2)
 			get_option(&str, &(cfg->monarch_timeout));

2.5.0