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

Groups > linux.kernel > #1355337 > unrolled thread

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

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 v14] x86, mce: Add memcpy_mcsafe() Mika Penttilä <mika.penttila@nextfour.com> - 2016-03-10 20:30 +0100
    RE: [PATCH v14] x86, mce: Add memcpy_mcsafe() "Luck, Tony" <tony.luck@intel.com> - 2016-03-10 20:40 +0100
      Re: [PATCH v14] x86, mce: Add memcpy_mcsafe() Tony Luck <tony.luck@gmail.com> - 2016-03-11 23:20 +0100
        Re: [PATCH v14] x86, mce: Add memcpy_mcsafe() Dan Williams <dan.j.williams@intel.com> - 2016-03-11 23:20 +0100
        Re: [PATCH v14] x86, mce: Add memcpy_mcsafe() Ingo Molnar <mingo@kernel.org> - 2016-03-12 18:20 +0100
          Re: [PATCH v14] x86, mce: Add memcpy_mcsafe() Linus Torvalds <torvalds@linux-foundation.org> - 2016-03-13 02:20 +0100
            Re: [PATCH v14] x86, mce: Add memcpy_mcsafe() Ingo Molnar <mingo@kernel.org> - 2016-03-13 10:30 +0100
              [PATCH] x86/mm, x86/mce: Fix return type/value for memcpy_mcsafe() Tony Luck <tony.luck@intel.com> - 2016-03-14 23:40 +0100
                [tip:x86/urgent] x86/mm, x86/mce: Fix return type/value for  memcpy_mcsafe() tip-bot for Tony Luck <tipbot@zytor.com> - 2016-03-16 09:40 +0100

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

On 18.02.2016 21:47, 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.
>
> 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"
>
> +
> +	/* 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
> +
>
But you return 0 == false for success and 1 == true for failure.

--Mika

[toc] | [next] | [standalone]

#1355346

> But you return 0 == false for success and 1 == true for failure.

Aaargh!  -ETOOMUCHSHELLSCRIPTPROGRAMMING

-Tony

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

#1356230

On Thu, Mar 10, 2016 at 11:37 AM, Luck, Tony <tony.luck@intel.com> wrote:
>> But you return 0 == false for success and 1 == true for failure.
>
> 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

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

#1356231

On Fri, Mar 11, 2016 at 2:10 PM, Tony Luck <tony.luck@gmail.com> wrote:
> On Thu, Mar 10, 2016 at 11:37 AM, Luck, Tony <tony.luck@intel.com> wrote:
>>> But you return 0 == false for success and 1 == true for failure.
>>
>> 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
>

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/

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

#1356487

* Tony Luck <tony.luck@gmail.com> wrote:

> On Thu, Mar 10, 2016 at 11:37 AM, Luck, Tony <tony.luck@intel.com> wrote:
> >> But you return 0 == false for success and 1 == true for failure.
> >
> > 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

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

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

#1356577

On Sat, Mar 12, 2016 at 9:16 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> Please use the copy_*_user() memory copying API semantics, which are: return
> negative code (-EFAULT) on error, 0 on success.

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

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

#1356658

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

> On Sat, Mar 12, 2016 at 9:16 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > Please use the copy_*_user() memory copying API semantics, which are: return
> > negative code (-EFAULT) on error, 0 on success.
> 
> 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. [...]

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

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

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:

    >> 2) Reverse the return values.

    (== 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

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

#1357658 — [PATCH] x86/mm, x86/mce: Fix return type/value for memcpy_mcsafe()

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ä <mika.penttila@nextfour.com>
Fixes: 92b0729c34ca ("x86/mm, x86/mce: Add memcpy_mcsafe()")
Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/include/asm/string_64.h | 4 ++--
 arch/x86/lib/memcpy_64.S         | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h
index ca6ba3607705..90dbbd9666d4 100644
--- a/arch/x86/include/asm/string_64.h
+++ b/arch/x86/include/asm/string_64.h
@@ -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__ */
 
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index cbb8ee5830ff..2ec0b0abbfaa 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -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

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

#1358704 — [tip:x86/urgent] x86/mm, x86/mce: Fix return type/value for memcpy_mcsafe()

Commit-ID:  cbf8b5a2b649a501758291cb4d4ba1e5711771ba
Gitweb:     http://git.kernel.org/tip/cbf8b5a2b649a501758291cb4d4ba1e5711771ba
Author:     Tony Luck <tony.luck@intel.com>
AuthorDate: Mon, 14 Mar 2016 15:33:39 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 16 Mar 2016 09:02:18 +0100

x86/mm, x86/mce: Fix return type/value for memcpy_mcsafe()

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ä <mika.penttila@nextfour.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: mika.penttila@nextfour.com
Fixes: 92b0729c34ca ("x86/mm, x86/mce: Add memcpy_mcsafe()")
Link: http://lkml.kernel.org/r/695f14233fa7a54fcac4406c706d7fec228e3f4c.1457993040.git.tony.luck@intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/string_64.h | 4 ++--
 arch/x86/lib/memcpy_64.S         | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/string_64.h b/arch/x86/include/asm/string_64.h
index ca6ba36..90dbbd9 100644
--- a/arch/x86/include/asm/string_64.h
+++ b/arch/x86/include/asm/string_64.h
@@ -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__ */
 
diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
index cbb8ee5..2ec0b0abb 100644
--- a/arch/x86/lib/memcpy_64.S
+++ b/arch/x86/lib/memcpy_64.S
@@ -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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web