Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1342254 > unrolled thread

Re: [PATCH v13] x86, mce: Add memcpy_trap()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v13] x86, mce: Add memcpy_trap() Tony Luck <tony.luck@gmail.com> - 2016-02-24 18:40 +0100
    Re: [PATCH v13] x86, mce: Add memcpy_trap() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-24 19:40 +0100
      Re: [PATCH v13] x86, mce: Add memcpy_trap() Tony Luck <tony.luck@gmail.com> - 2016-02-24 20:30 +0100
        Re: [PATCH v13] x86, mce: Add memcpy_trap() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-24 20:40 +0100
          Re: [PATCH v13] x86, mce: Add memcpy_trap() Ingo Molnar <mingo@kernel.org> - 2016-02-25 10:00 +0100
            Re: [PATCH v13] x86, mce: Add memcpy_trap() "Luck, Tony" <tony.luck@intel.com> - 2016-02-25 20:40 +0100
              Re: [PATCH v13] x86, mce: Add memcpy_trap() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-25 21:40 +0100
                Re: [PATCH v13] x86, mce: Add memcpy_trap() Andy Lutomirski <luto@amacapital.net> - 2016-02-25 23:20 +0100
                  [PATCH v14] x86, mce: Add memcpy_mcsafe() Tony Luck <tony.luck@intel.com> - 2016-02-25 23:50 +0100
                    Re: [PATCH v14] x86, mce: Add memcpy_mcsafe() "Luck, Tony" <tony.luck@intel.com> - 2016-03-02 21:50 +0100
                  Re: [PATCH v13] x86, mce: Add memcpy_trap() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-26 02:00 +0100
                    Re: [PATCH v13] x86, mce: Add memcpy_trap() Andy Lutomirski <luto@amacapital.net> - 2016-02-26 02:30 +0100
                      Re: [PATCH v13] x86, mce: Add memcpy_trap() Linus Torvalds <torvalds@linux-foundation.org> - 2016-02-26 03:40 +0100

#1342254 — Re: [PATCH v13] x86, mce: Add memcpy_trap()

On Fri, Feb 19, 2016 at 9:53 AM, Luck, Tony <tony.luck@intel.com> wrote:
> 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.
>
> Reviewed-by: Borislav Petkov <bp@suse.de>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> 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(+)

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

[toc] | [next] | [standalone]

#1342300

On Feb 24, 2016 09:38, "Tony Luck" <tony.luck@gmail.com> wrote:
>
> 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)

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

[toc] | [prev] | [next] | [standalone]

#1342365

On Wed, Feb 24, 2016 at 10:35 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Feb 24, 2016 09:38, "Tony Luck" <tony.luck@gmail.com> wrote:
>>
>> 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)
>
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1342372

On Wed, Feb 24, 2016 at 11:27 AM, Tony Luck <tony.luck@gmail.com> wrote:
>
> 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

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

[toc] | [prev] | [next] | [standalone]

#1342996

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Wed, Feb 24, 2016 at 11:27 AM, Tony Luck <tony.luck@gmail.com> wrote:
> >
> > 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
> 
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1343432

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

diff --git a/arch/x86/lib/copy_user_64.S b/arch/x86/lib/copy_user_64.S
index 982ce34f4a9b..e31d8964ac09 100644
--- a/arch/x86/lib/copy_user_64.S
+++ b/arch/x86/lib/copy_user_64.S
@@ -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"
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index 0a42327a59d7..c377a70474e0 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -6,6 +6,8 @@
  * Copyright 2002 Andi Kleen <ak@suse.de>
  */
 #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;
+}

[toc] | [prev] | [next] | [standalone]

#1343465

On Thu, Feb 25, 2016 at 11:33 AM, Luck, Tony <tony.luck@intel.com> wrote:
> 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]

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

[toc] | [prev] | [next] | [standalone]

#1343524

On Feb 25, 2016 12:39 PM, "Linus Torvalds"
<torvalds@linux-foundation.org> wrote:
>
> 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.

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.

>

> 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

[toc] | [prev] | [next] | [standalone]

#1343546 — [PATCH v14] x86, mce: Add memcpy_mcsafe()

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.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---

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(+)

diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h
index ff8b9a17dc4b..ca6ba3607705 100644
--- a/arch/x86/include/asm/string_64.h
+++ b/arch/x86/include/asm/string_64.h
@@ -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 */
diff --git a/arch/x86/kernel/x8664_ksyms_64.c b/arch/x86/kernel/x8664_ksyms_64.c
index a0695be19864..cd05942bc918 100644
--- a/arch/x86/kernel/x8664_ksyms_64.c
+++ b/arch/x86/kernel/x8664_ksyms_64.c
@@ -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);
 
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index 16698bba87de..7d37641ada5b 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -177,3 +177,120 @@ ENTRY(memcpy_orig)
 .Lend:
 	retq
 ENDPROC(memcpy_orig)
+
+#ifndef CONFIG_UML
+/*
+ * memcpy_mcsafe - memory copy with machine check exception handling
+ * Note that we only catch machine checks when reading the source addresses.
+ * Writes to target are posted and don't generate machine checks.
+ */
+ENTRY(memcpy_mcsafe)
+	cmpl $8, %edx
+	/* Less than 8 bytes? Go to byte copy loop */
+	jb .L_no_whole_words
+
+	/* Check for bad alignment of source */
+	testl $7, %esi
+	/* Already aligned */
+	jz .L_8byte_aligned
+
+	/* Copy one byte at a time until source is 8-byte aligned */
+	movl %esi, %ecx
+	andl $7, %ecx
+	subl $8, %ecx
+	negl %ecx
+	subl %ecx, %edx
+.L_copy_leading_bytes:
+	movb (%rsi), %al
+	movb %al, (%rdi)
+	incq %rsi
+	incq %rdi
+	decl %ecx
+	jnz .L_copy_leading_bytes
+
+.L_8byte_aligned:
+	/* Figure out how many whole cache lines (64-bytes) to copy */
+	movl %edx, %ecx
+	andl $63, %edx
+	shrl $6, %ecx
+	jz .L_no_whole_cache_lines
+
+	/* Loop copying whole cache lines */
+.L_cache_w0: movq (%rsi), %r8
+.L_cache_w1: movq 1*8(%rsi), %r9
+.L_cache_w2: movq 2*8(%rsi), %r10
+.L_cache_w3: movq 3*8(%rsi), %r11
+	movq %r8, (%rdi)
+	movq %r9, 1*8(%rdi)
+	movq %r10, 2*8(%rdi)
+	movq %r11, 3*8(%rdi)
+.L_cache_w4: movq 4*8(%rsi), %r8
+.L_cache_w5: movq 5*8(%rsi), %r9
+.L_cache_w6: movq 6*8(%rsi), %r10
+.L_cache_w7: movq 7*8(%rsi), %r11
+	movq %r8, 4*8(%rdi)
+	movq %r9, 5*8(%rdi)
+	movq %r10, 6*8(%rdi)
+	movq %r11, 7*8(%rdi)
+	leaq 64(%rsi), %rsi
+	leaq 64(%rdi), %rdi
+	decl %ecx
+	jnz .L_cache_w0
+
+	/* Are there any trailing 8-byte words? */
+.L_no_whole_cache_lines:
+	movl %edx, %ecx
+	andl $7, %edx
+	shrl $3, %ecx
+	jz .L_no_whole_words
+
+	/* Copy trailing words */
+.L_copy_trailing_words:
+	movq (%rsi), %r8
+	mov %r8, (%rdi)
+	leaq 8(%rsi), %rsi
+	leaq 8(%rdi), %rdi
+	decl %ecx
+	jnz .L_copy_trailing_words
+
+	/* Any trailing bytes? */
+.L_no_whole_words:
+	andl %edx, %edx
+	jz .L_done_memcpy_trap
+
+	/* Copy trailing bytes */
+	movl %edx, %ecx
+.L_copy_trailing_bytes:
+	movb (%rsi), %al
+	movb %al, (%rdi)
+	incq %rsi
+	incq %rdi
+	decl %ecx
+	jnz .L_copy_trailing_bytes
+
+	/* Copy successful. Return true */
+.L_done_memcpy_trap:
+	xorq %rax, %rax
+	ret
+ENDPROC(memcpy_mcsafe)
+
+	.section .fixup, "ax"
+	/* Return false for any failure */
+.L_memcpy_mcsafe_fail:
+	mov	$1, %rax
+	ret
+
+	.previous
+
+	_ASM_EXTABLE_FAULT(.L_copy_leading_bytes, .L_memcpy_mcsafe_fail)
+	_ASM_EXTABLE_FAULT(.L_cache_w0, .L_memcpy_mcsafe_fail)
+	_ASM_EXTABLE_FAULT(.L_cache_w1, .L_memcpy_mcsafe_fail)
+	_ASM_EXTABLE_FAULT(.L_cache_w3, .L_memcpy_mcsafe_fail)
+	_ASM_EXTABLE_FAULT(.L_cache_w3, .L_memcpy_mcsafe_fail)
+	_ASM_EXTABLE_FAULT(.L_cache_w4, .L_memcpy_mcsafe_fail)
+	_ASM_EXTABLE_FAULT(.L_cache_w5, .L_memcpy_mcsafe_fail)
+	_ASM_EXTABLE_FAULT(.L_cache_w6, .L_memcpy_mcsafe_fail)
+	_ASM_EXTABLE_FAULT(.L_cache_w7, .L_memcpy_mcsafe_fail)
+	_ASM_EXTABLE_FAULT(.L_copy_trailing_words, .L_memcpy_mcsafe_fail)
+	_ASM_EXTABLE_FAULT(.L_copy_trailing_bytes, .L_memcpy_mcsafe_fail)
+#endif
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1348469 — Re: [PATCH v14] x86, mce: Add memcpy_mcsafe()

On Thu, Feb 18, 2016 at 11:47:26AM -0800, Tony Luck wrote:
> 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.

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

[toc] | [prev] | [next] | [standalone]

#1343643

On Thu, Feb 25, 2016 at 2:11 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> 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.

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.

> 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.

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

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

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.

> 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.

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.

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

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

[toc] | [prev] | [next] | [standalone]

#1343712

On Thu, Feb 25, 2016 at 4:58 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Feb 25, 2016 at 2:11 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>
>> 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.
>
> So?
>

[...]

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

[toc] | [prev] | [next] | [standalone]

#1343742

On Thu, Feb 25, 2016 at 5:19 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> 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.

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.

> 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.

Agreed.

> 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?

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.

>  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.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web