Re: [PATCH] powerpc: Don't use asm goto for put_user() with GCC 4.9

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

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);				\
>