* [PATCH] powerpc: Don't use asm goto for put_user() with GCC 4.9
@ 2020-11-03 13:29 Michael Ellerman
2020-11-03 14:43 ` Christophe Leroy
0 siblings, 1 reply; 6+ messages in thread
From: Michael Ellerman @ 2020-11-03 13:29 UTC (permalink / raw)
To: linuxppc-dev; +Cc: schwab
Andreas reported that commit ee0a49a6870e ("powerpc/uaccess: Switch
__put_user_size_allowed() to __put_user_asm_goto()") broke
CLONE_CHILD_SETTID.
Further inspection showed that the put_user() in schedule_tail() was
missing entirely, the store not emitted by the compiler.
<.schedule_tail>:
mflr r0
std r0,16(r1)
stdu r1,-112(r1)
bl <.finish_task_switch>
ld r9,2496(r3)
cmpdi cr7,r9,0
bne cr7,<.schedule_tail+0x60>
ld r3,392(r13)
ld r9,1392(r3)
cmpdi cr7,r9,0
beq cr7,<.schedule_tail+0x3c>
li r4,0
li r5,0
bl <.__task_pid_nr_ns>
nop
bl <.calculate_sigpending>
nop
addi r1,r1,112
ld r0,16(r1)
mtlr r0
blr
nop
nop
nop
bl <.__balance_callback>
b <.schedule_tail+0x1c>
Notice there are no stores other than to the stack. There should be a
stw in there for the store to current->set_child_tid.
This is only seen with GCC 4.9 era compilers (tested with 4.9.3 and
4.9.4), and only when CONFIG_PPC_KUAP is disabled.
When CONFIG_PPC_KUAP=y, the memory clobber that's part of the isync()
and mtspr() inlined via allow_user_access() seems to be enough to
avoid the bug.
For now though let's just not use asm goto with GCC 4.9, to avoid this
bug and any other issues we haven't noticed yet. Possibly in future we
can find a smaller workaround.
This is basically a revert of commit ee0a49a6870e ("powerpc/uaccess:
Switch __put_user_size_allowed() to __put_user_asm_goto()") and commit
7fdf966bed15 ("powerpc/uaccess: Remove __put_user_asm() and
__put_user_asm2()"), but only for GCC 4.9.
With this applied the code generation looks more like it will work:
<.schedule_tail>:
mflr r0
std r31,-8(r1)
std r0,16(r1)
stdu r1,-144(r1)
std r3,112(r1)
bl <._mcount>
nop
ld r3,112(r1)
bl <.finish_task_switch>
ld r9,2624(r3)
cmpdi cr7,r9,0
bne cr7,<.schedule_tail+0xa0>
ld r3,2408(r13)
ld r31,1856(r3)
cmpdi cr7,r31,0
beq cr7,<.schedule_tail+0x80>
li r4,0
li r5,0
bl <.__task_pid_nr_ns>
nop
li r9,-1
clrldi r9,r9,12
cmpld cr7,r31,r9
bgt cr7,<.schedule_tail+0x80>
lis r9,16
rldicr r9,r9,32,31
subf r9,r31,r9
cmpldi cr7,r9,3
ble cr7,<.schedule_tail+0x80>
li r9,0
stw r3,0(r31) <-- stw
nop
bl <.calculate_sigpending>
nop
addi r1,r1,144
ld r0,16(r1)
ld r31,-8(r1)
mtlr r0
blr
nop
bl <.__balance_callback>
b <.schedule_tail+0x30>
Fixes: ee0a49a6870e ("powerpc/uaccess: Switch __put_user_size_allowed() to __put_user_asm_goto()")
Reported-by: Andreas Schwab <schwab@linux-m68k.org>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
arch/powerpc/include/asm/uaccess.h | 48 ++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index ef5bbb705c08..526a6658946b 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -110,6 +110,52 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
extern long __put_user_bad(void);
+#if defined(GCC_VERSION) && GCC_VERSION < 50000
+#define __put_user_asm(x, addr, err, op) \
+ __asm__ __volatile__( \
+ "1: " op "%U2%X2 %1,%2 # put_user\n" \
+ "2:\n" \
+ ".section .fixup,\"ax\"\n" \
+ "3: li %0,%3\n" \
+ " b 2b\n" \
+ ".previous\n" \
+ EX_TABLE(1b, 3b) \
+ : "=r" (err) \
+ : "r" (x), "m<>" (*addr), "i" (-EFAULT), "0" (err))
+
+#ifdef __powerpc64__
+#define __put_user_asm2(x, ptr, retval) \
+ __put_user_asm(x, ptr, retval, "std")
+#else /* __powerpc64__ */
+#define __put_user_asm2(x, addr, err) \
+ __asm__ __volatile__( \
+ "1: stw%X2 %1,%2\n" \
+ "2: stw%X2 %L1,%L2\n" \
+ "3:\n" \
+ ".section .fixup,\"ax\"\n" \
+ "4: li %0,%3\n" \
+ " b 3b\n" \
+ ".previous\n" \
+ EX_TABLE(1b, 4b) \
+ EX_TABLE(2b, 4b) \
+ : "=r" (err) \
+ : "r" (x), "m" (*addr), "i" (-EFAULT), "0" (err))
+#endif /* __powerpc64__ */
+
+#define __put_user_size_allowed(x, ptr, size, retval) \
+do { \
+ retval = 0; \
+ switch (size) { \
+ case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
+ case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
+ case 4: __put_user_asm(x, ptr, retval, "stw"); break; \
+ case 8: __put_user_asm2(x, ptr, retval); break; \
+ default: __put_user_bad(); \
+ } \
+} while (0)
+
+#else
+
#define __put_user_size_allowed(x, ptr, size, retval) \
do { \
__label__ __pu_failed; \
@@ -122,6 +168,8 @@ __pu_failed: \
retval = -EFAULT; \
} while (0)
+#endif /* GCC_VERSION */
+
#define __put_user_size(x, ptr, size, retval) \
do { \
allow_write_to_user(ptr, size); \
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Don't use asm goto for put_user() with GCC 4.9
2020-11-03 13:29 [PATCH] powerpc: Don't use asm goto for put_user() with GCC 4.9 Michael Ellerman
@ 2020-11-03 14:43 ` Christophe Leroy
2020-11-03 17:04 ` Christophe Leroy
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Christophe Leroy @ 2020-11-03 14:43 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: schwab
Le 03/11/2020 à 14:29, Michael Ellerman a écrit :
> Andreas reported that commit ee0a49a6870e ("powerpc/uaccess: Switch
> __put_user_size_allowed() to __put_user_asm_goto()") broke
> CLONE_CHILD_SETTID.
>
> Further inspection showed that the put_user() in schedule_tail() was
> missing entirely, the store not emitted by the compiler.
>
>
> Notice there are no stores other than to the stack. There should be a
> stw in there for the store to current->set_child_tid.
>
> This is only seen with GCC 4.9 era compilers (tested with 4.9.3 and
> 4.9.4), and only when CONFIG_PPC_KUAP is disabled.
>
> When CONFIG_PPC_KUAP=y, the memory clobber that's part of the isync()
> and mtspr() inlined via allow_user_access() seems to be enough to
> avoid the bug.
>
> For now though let's just not use asm goto with GCC 4.9, to avoid this
> bug and any other issues we haven't noticed yet. Possibly in future we
> can find a smaller workaround.
Is that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670 ?
Should we use asm_volatile_goto() defined in include/linux/compiler-gcc.h ?
Christophe
>
> This is basically a revert of commit ee0a49a6870e ("powerpc/uaccess:
> Switch __put_user_size_allowed() to __put_user_asm_goto()") and commit
> 7fdf966bed15 ("powerpc/uaccess: Remove __put_user_asm() and
> __put_user_asm2()"), but only for GCC 4.9.
>
> With this applied the code generation looks more like it will work:
>
>
> Fixes: ee0a49a6870e ("powerpc/uaccess: Switch __put_user_size_allowed() to __put_user_asm_goto()")
> Reported-by: Andreas Schwab <schwab@linux-m68k.org>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> ---
> arch/powerpc/include/asm/uaccess.h | 48 ++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index ef5bbb705c08..526a6658946b 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -110,6 +110,52 @@ static inline bool __access_ok(unsigned long addr, unsigned long size)
>
> extern long __put_user_bad(void);
>
> +#if defined(GCC_VERSION) && GCC_VERSION < 50000
> +#define __put_user_asm(x, addr, err, op) \
> + __asm__ __volatile__( \
> + "1: " op "%U2%X2 %1,%2 # put_user\n" \
> + "2:\n" \
> + ".section .fixup,\"ax\"\n" \
> + "3: li %0,%3\n" \
> + " b 2b\n" \
> + ".previous\n" \
> + EX_TABLE(1b, 3b) \
> + : "=r" (err) \
> + : "r" (x), "m<>" (*addr), "i" (-EFAULT), "0" (err))
> +
> +#ifdef __powerpc64__
> +#define __put_user_asm2(x, ptr, retval) \
> + __put_user_asm(x, ptr, retval, "std")
> +#else /* __powerpc64__ */
> +#define __put_user_asm2(x, addr, err) \
> + __asm__ __volatile__( \
> + "1: stw%X2 %1,%2\n" \
> + "2: stw%X2 %L1,%L2\n" \
> + "3:\n" \
> + ".section .fixup,\"ax\"\n" \
> + "4: li %0,%3\n" \
> + " b 3b\n" \
> + ".previous\n" \
> + EX_TABLE(1b, 4b) \
> + EX_TABLE(2b, 4b) \
> + : "=r" (err) \
> + : "r" (x), "m" (*addr), "i" (-EFAULT), "0" (err))
> +#endif /* __powerpc64__ */
> +
> +#define __put_user_size_allowed(x, ptr, size, retval) \
> +do { \
> + retval = 0; \
> + switch (size) { \
> + case 1: __put_user_asm(x, ptr, retval, "stb"); break; \
> + case 2: __put_user_asm(x, ptr, retval, "sth"); break; \
> + case 4: __put_user_asm(x, ptr, retval, "stw"); break; \
> + case 8: __put_user_asm2(x, ptr, retval); break; \
> + default: __put_user_bad(); \
> + } \
> +} while (0)
> +
> +#else
> +
> #define __put_user_size_allowed(x, ptr, size, retval) \
> do { \
> __label__ __pu_failed; \
> @@ -122,6 +168,8 @@ __pu_failed: \
> retval = -EFAULT; \
> } while (0)
>
> +#endif /* GCC_VERSION */
> +
> #define __put_user_size(x, ptr, size, retval) \
> do { \
> allow_write_to_user(ptr, size); \
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Don't use asm goto for put_user() with GCC 4.9
2020-11-03 14:43 ` Christophe Leroy
@ 2020-11-03 17:04 ` Christophe Leroy
2020-11-03 18:58 ` Segher Boessenkool
2020-11-04 11:04 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Christophe Leroy @ 2020-11-03 17:04 UTC (permalink / raw)
To: Michael Ellerman, linuxppc-dev; +Cc: schwab
Quoting Christophe Leroy <christophe.leroy@csgroup.eu>:
> Le 03/11/2020 à 14:29, Michael Ellerman a écrit :
>> Andreas reported that commit ee0a49a6870e ("powerpc/uaccess: Switch
>> __put_user_size_allowed() to __put_user_asm_goto()") broke
>> CLONE_CHILD_SETTID.
>>
>> Further inspection showed that the put_user() in schedule_tail() was
>> missing entirely, the store not emitted by the compiler.
>>
>
>>
>> Notice there are no stores other than to the stack. There should be a
>> stw in there for the store to current->set_child_tid.
>>
>> This is only seen with GCC 4.9 era compilers (tested with 4.9.3 and
>> 4.9.4), and only when CONFIG_PPC_KUAP is disabled.
>>
>> When CONFIG_PPC_KUAP=y, the memory clobber that's part of the isync()
>> and mtspr() inlined via allow_user_access() seems to be enough to
>> avoid the bug.
>>
>> For now though let's just not use asm goto with GCC 4.9, to avoid this
>> bug and any other issues we haven't noticed yet. Possibly in future we
>> can find a smaller workaround.
>
> Is that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670 ?
>
> Should we use asm_volatile_goto() defined in include/linux/compiler-gcc.h ?
It seems to be OK with asm_volatile_goto() with GCC 4.9, and it is
already what is used in our asm/jump_label.h
diff --git a/arch/powerpc/include/asm/uaccess.h
b/arch/powerpc/include/asm/uaccess.h
index ef5bbb705c08..501c9a79038c 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -178,7 +178,7 @@ do { \
* are no aliasing issues.
*/
#define __put_user_asm_goto(x, addr, label, op) \
- asm volatile goto( \
+ asm_volatile_goto( \
"1: " op "%U1%X1 %0,%1 # put_user\n" \
EX_TABLE(1b, %l2) \
: \
@@ -191,7 +191,7 @@ do { \
__put_user_asm_goto(x, ptr, label, "std")
#else /* __powerpc64__ */
#define __put_user_asm2_goto(x, addr, label) \
- asm volatile goto( \
+ asm_volatile_goto( \
"1: stw%X1 %0, %1\n" \
"2: stw%X1 %L0, %L1\n" \
EX_TABLE(1b, %l2) \
---
Christophe
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Don't use asm goto for put_user() with GCC 4.9
2020-11-03 14:43 ` Christophe Leroy
2020-11-03 17:04 ` Christophe Leroy
@ 2020-11-03 18:58 ` Segher Boessenkool
2020-11-03 19:22 ` Christophe Leroy
2020-11-04 11:04 ` Michael Ellerman
2 siblings, 1 reply; 6+ messages in thread
From: Segher Boessenkool @ 2020-11-03 18:58 UTC (permalink / raw)
To: Christophe Leroy; +Cc: schwab, linuxppc-dev
On Tue, Nov 03, 2020 at 03:43:55PM +0100, Christophe Leroy wrote:
> Le 03/11/2020 à 14:29, Michael Ellerman a écrit :
> >For now though let's just not use asm goto with GCC 4.9, to avoid this
> >bug and any other issues we haven't noticed yet. Possibly in future we
> >can find a smaller workaround.
>
> Is that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670 ?
That was fixed in 4.8.1 (and all 4.9), so probably not.
Segher
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Don't use asm goto for put_user() with GCC 4.9
2020-11-03 18:58 ` Segher Boessenkool
@ 2020-11-03 19:22 ` Christophe Leroy
0 siblings, 0 replies; 6+ messages in thread
From: Christophe Leroy @ 2020-11-03 19:22 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: schwab, linuxppc-dev
Le 03/11/2020 à 19:58, Segher Boessenkool a écrit :
> On Tue, Nov 03, 2020 at 03:43:55PM +0100, Christophe Leroy wrote:
>> Le 03/11/2020 à 14:29, Michael Ellerman a écrit :
>>> For now though let's just not use asm goto with GCC 4.9, to avoid this
>>> bug and any other issues we haven't noticed yet. Possibly in future we
>>> can find a smaller workaround.
>>
>> Is that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670 ?
>
> That was fixed in 4.8.1 (and all 4.9), so probably not.
>
Ok.
Regardless, using "asm_volatile_goto()" instead of "asm volatile goto()" fixes the issue it seems.
Christophe
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] powerpc: Don't use asm goto for put_user() with GCC 4.9
2020-11-03 14:43 ` Christophe Leroy
2020-11-03 17:04 ` Christophe Leroy
2020-11-03 18:58 ` Segher Boessenkool
@ 2020-11-04 11:04 ` Michael Ellerman
2 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2020-11-04 11:04 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev; +Cc: schwab
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Le 03/11/2020 à 14:29, Michael Ellerman a écrit :
>> Andreas reported that commit ee0a49a6870e ("powerpc/uaccess: Switch
>> __put_user_size_allowed() to __put_user_asm_goto()") broke
>> CLONE_CHILD_SETTID.
>>
>> Further inspection showed that the put_user() in schedule_tail() was
>> missing entirely, the store not emitted by the compiler.
>>
>> Notice there are no stores other than to the stack. There should be a
>> stw in there for the store to current->set_child_tid.
>>
>> This is only seen with GCC 4.9 era compilers (tested with 4.9.3 and
>> 4.9.4), and only when CONFIG_PPC_KUAP is disabled.
>>
>> When CONFIG_PPC_KUAP=y, the memory clobber that's part of the isync()
>> and mtspr() inlined via allow_user_access() seems to be enough to
>> avoid the bug.
>>
>> For now though let's just not use asm goto with GCC 4.9, to avoid this
>> bug and any other issues we haven't noticed yet. Possibly in future we
>> can find a smaller workaround.
>
> Is that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58670 ?
>
> Should we use asm_volatile_goto() defined in include/linux/compiler-gcc.h ?
Ugh. I knew of that work around, but thought we'd dropped it when we
moved up to GCC 4.9. I should have looked more closely.
I'll send a patch to switch to it.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-04 11:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03 13:29 [PATCH] powerpc: Don't use asm goto for put_user() with GCC 4.9 Michael Ellerman
2020-11-03 14:43 ` Christophe Leroy
2020-11-03 17:04 ` Christophe Leroy
2020-11-03 18:58 ` Segher Boessenkool
2020-11-03 19:22 ` Christophe Leroy
2020-11-04 11:04 ` Michael Ellerman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.