[PATCH] powerpc/32: Avoid miscompilation w/GCC 4.6.3 - don't inline copy_to/from_user()

[PATCH] powerpc/32: Avoid miscompilation w/GCC 4.6.3 - don't inline copy_to/from_user()

[Michael Ellerman]

Larry Finger reported that his Powerbook G4 was no longer booting with v4.12-rc,
userspace was up but giving weird errors such as:

  udevd[64]: starting version 175
  udevd[64]: Unable to receive ctrl message: Bad address.
  modprobe: chdir(4.12-rc1): No such file or directory

He bisected the problem to commit 3448890c32c3 ("powerpc: get rid of zeroing,
switch to RAW_COPY_USER").

Al identified that the problem is actually a miscompilation by GCC 4.6.3, which
is exposed by the above commit.

Al also pointed out that inlining copy_to/from_user() is probably of little or
no benefit, which is correct. Using Anton's copy_to_user benchmark, with a
pathological single byte copy, we see a small increase in performance
by *removing* inlining:

  Before (inlined):
  # time ./copy_to_user -w -l 1 -i 10000000	( x 3 )
  real	0m22.063s
  real	0m22.059s
  real	0m22.076s

  After:
  # time ./copy_to_user -w -l 1 -i 10000000	( x 3 )
  real	0m21.325s
  real	0m21.299s
  real	0m21.364s

So as a small performance improvement and to avoid the miscompilation, drop
inlining copy_to/from_user() on 32-bit.

Fixes: 3448890c32c3 ("powerpc: get rid of zeroing, switch to RAW_COPY_USER")
Reported-by: Larry Finger <Larry.Finger@lwfinger.net>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/uaccess.h | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 5c0d8a8cdae5..41e88d3ce36b 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -267,13 +267,7 @@ do {								\
 extern unsigned long __copy_tofrom_user(void __user *to,
 		const void __user *from, unsigned long size);
 
-#ifndef __powerpc64__
-
-#define INLINE_COPY_FROM_USER
-#define INLINE_COPY_TO_USER
-
-#else /* __powerpc64__ */
-
+#ifdef __powerpc64__
 static inline unsigned long
 raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
 {
-- 
2.7.4

RE: [PATCH] powerpc/32: Avoid miscompilation w/GCC 4.6.3 - don't inline copy_to/from_user()

[David Laight]

From: Michael Ellerman
> Sent: 26 June 2017 14:34
..
> Al also pointed out that inlining copy_to/from_user() is probably of litt=
le or
> no benefit, which is correct
...

I was a bit horrified at the x86-64 versions of copy_to/from_user() as well=
.
With code that (tries to) error kernel pointers that cross stack frame
boundaries I'm fairly sure they expand to a lot of code.

I also suspect the cost of that kernel address check is not insignificant
especially if it has to walk down several stack frames.
(Never mind what happens to code compiled without stack frames.)

	David

Re: powerpc/32: Avoid miscompilation w/GCC 4.6.3 - don't inline copy_to/from_user()

[Michael Ellerman]

On Mon, 2017-06-26 at 13:34:15 UTC, Michael Ellerman wrote:
> Larry Finger reported that his Powerbook G4 was no longer booting with v4.12-rc,
> userspace was up but giving weird errors such as:
> 
>   udevd[64]: starting version 175
>   udevd[64]: Unable to receive ctrl message: Bad address.
>   modprobe: chdir(4.12-rc1): No such file or directory
> 
> He bisected the problem to commit 3448890c32c3 ("powerpc: get rid of zeroing,
> switch to RAW_COPY_USER").
> 
> Al identified that the problem is actually a miscompilation by GCC 4.6.3, which
> is exposed by the above commit.
> 
> Al also pointed out that inlining copy_to/from_user() is probably of little or
> no benefit, which is correct. Using Anton's copy_to_user benchmark, with a
> pathological single byte copy, we see a small increase in performance
> by *removing* inlining:
> 
>   Before (inlined):
>   # time ./copy_to_user -w -l 1 -i 10000000	( x 3 )
>   real	0m22.063s
>   real	0m22.059s
>   real	0m22.076s
> 
>   After:
>   # time ./copy_to_user -w -l 1 -i 10000000	( x 3 )
>   real	0m21.325s
>   real	0m21.299s
>   real	0m21.364s
> 
> So as a small performance improvement and to avoid the miscompilation, drop
> inlining copy_to/from_user() on 32-bit.
> 
> Fixes: 3448890c32c3 ("powerpc: get rid of zeroing, switch to RAW_COPY_USER")
> Reported-by: Larry Finger <Larry.Finger@lwfinger.net>
> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

Applied to powerpc fixes.

https://git.kernel.org/powerpc/c/d6bd8194e2867e85ac2de63486d3b8

cheers