Re: [patch V5 04/12] powerpc/uaccess: Use unsafe wrappers for ASM GOTO

From: Christophe Leroy <christophe.leroy@csgroup.eu>
To: Thomas Gleixner <tglx@linutronix.de>,
	LKML <linux-kernel@vger.kernel.org>
Cc: "Madhavan Srinivasan" <maddy@linux.ibm.com>,
	"Michael Ellerman" <mpe@ellerman.id.au>,
	"Nicholas Piggin" <npiggin@gmail.com>,
	linuxppc-dev@lists.ozlabs.org,
	"kernel test robot" <lkp@intel.com>,
	"Russell King" <linux@armlinux.org.uk>,
	linux-arm-kernel@lists.infradead.org,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	x86@kernel.org, "Paul Walmsley" <pjw@kernel.org>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	linux-riscv@lists.infradead.org,
	"Heiko Carstens" <hca@linux.ibm.com>,
	"Christian Borntraeger" <borntraeger@linux.ibm.com>,
	"Sven Schnelle" <svens@linux.ibm.com>,
	linux-s390@vger.kernel.org,
	"Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"David Laight" <david.laight.linux@gmail.com>,
	"Julia Lawall" <Julia.Lawall@inria.fr>,
	"Nicolas Palix" <nicolas.palix@imag.fr>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Darren Hart" <dvhart@infradead.org>,
	"Davidlohr Bueso" <dave@stgolabs.net>,
	"André Almeida" <andrealmeid@igalia.com>,
	"Alexander Viro" <viro@zeniv.linux.org.uk>,
	"Christian Brauner" <brauner@kernel.org>,
	"Jan Kara" <jack@suse.cz>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [patch V5 04/12] powerpc/uaccess: Use unsafe wrappers for ASM GOTO
Date: Tue, 4 Nov 2025 07:15:30 +0100	[thread overview]
Message-ID: <f8872b02-d7f5-421d-8699-5d27987c6e1f@csgroup.eu> (raw)
In-Reply-To: <20251027083745.356628509@linutronix.de>

Le 27/10/2025 à 09:43, Thomas Gleixner a écrit :
> ASM GOTO is miscompiled by GCC when it is used inside a auto cleanup scope:
> 
> bool foo(u32 __user *p, u32 val)
> {
> 	scoped_guard(pagefault)
> 		unsafe_put_user(val, p, efault);
> 	return true;
> efault:
> 	return false;
> }
> 
> It ends up leaking the pagefault disable counter in the fault path. clang
> at least fails the build.

Confirmed on powerpc:

Before the patch

000001e8 <foo>:
  1e8:	81 22 05 24 	lwz     r9,1316(r2)
  1ec:	39 29 00 01 	addi    r9,r9,1
  1f0:	91 22 05 24 	stw     r9,1316(r2)
  1f4:	90 83 00 00 	stw     r4,0(r3)
  1f8:	81 22 05 24 	lwz     r9,1316(r2)
  1fc:	38 60 00 01 	li      r3,1
  200:	39 29 ff ff 	addi    r9,r9,-1
  204:	91 22 05 24 	stw     r9,1316(r2)
  208:	4e 80 00 20 	blr
  20c:	38 60 00 00 	li      r3,0
  210:	4e 80 00 20 	blr

00000000 <__ex_table>:
         ...
                         20: R_PPC_REL32 .text+0x1f4
                         24: R_PPC_REL32 .text+0x20c

After the patch

000001e8 <foo>:
  1e8:	81 22 05 24 	lwz     r9,1316(r2)
  1ec:	39 29 00 01 	addi    r9,r9,1
  1f0:	91 22 05 24 	stw     r9,1316(r2)
  1f4:	90 83 00 00 	stw     r4,0(r3)
  1f8:	81 22 05 24 	lwz     r9,1316(r2)
  1fc:	38 60 00 01 	li      r3,1
  200:	39 29 ff ff 	addi    r9,r9,-1
  204:	91 22 05 24 	stw     r9,1316(r2)
  208:	4e 80 00 20 	blr
  20c:	81 22 05 24 	lwz     r9,1316(r2)
  210:	38 60 00 00 	li      r3,0
  214:	39 29 ff ff 	addi    r9,r9,-1
  218:	91 22 05 24 	stw     r9,1316(r2)
  21c:	4e 80 00 20 	blr

00000000 <__ex_table>:
         ...
                         20: R_PPC_REL32 .text+0x1f4
                         24: R_PPC_REL32 .text+0x20c

> 
> Rename unsafe_*_user() to arch_unsafe_*_user() which makes the generic
> uaccess header wrap it with a local label that makes both compilers emit
> correct code. Same for the kernel_nofault() variants.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Madhavan Srinivasan <maddy@linux.ibm.com>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Nicholas Piggin <npiggin@gmail.com>
> Cc: Christophe Leroy <christophe.leroy@csgroup.eu>
> Cc: linuxppc-dev@lists.ozlabs.org

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   arch/powerpc/include/asm/uaccess.h |    8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -451,7 +451,7 @@ user_write_access_begin(const void __use
>   #define user_write_access_begin	user_write_access_begin
>   #define user_write_access_end		prevent_current_write_to_user
>   
> -#define unsafe_get_user(x, p, e) do {					\
> +#define arch_unsafe_get_user(x, p, e) do {			\
>   	__long_type(*(p)) __gu_val;				\
>   	__typeof__(*(p)) __user *__gu_addr = (p);		\
>   								\
> @@ -459,7 +459,7 @@ user_write_access_begin(const void __use
>   	(x) = (__typeof__(*(p)))__gu_val;			\
>   } while (0)
>   
> -#define unsafe_put_user(x, p, e) \
> +#define arch_unsafe_put_user(x, p, e)				\
>   	__put_user_size_goto((__typeof__(*(p)))(x), (p), sizeof(*(p)), e)
>   
>   #define unsafe_copy_from_user(d, s, l, e) \
> @@ -504,11 +504,11 @@ do {									\
>   		unsafe_put_user(*(u8*)(_src + _i), (u8 __user *)(_dst + _i), e); \
>   } while (0)
>   
> -#define __get_kernel_nofault(dst, src, type, err_label)			\
> +#define arch_get_kernel_nofault(dst, src, type, err_label)		\
>   	__get_user_size_goto(*((type *)(dst)),				\
>   		(__force type __user *)(src), sizeof(type), err_label)
>   
> -#define __put_kernel_nofault(dst, src, type, err_label)			\
> +#define arch_put_kernel_nofault(dst, src, type, err_label)		\
>   	__put_user_size_goto(*((type *)(src)),				\
>   		(__force type __user *)(dst), sizeof(type), err_label)
>   
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv