Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()

[Linus Torvalds <torvalds@linux-foundation.org>]

[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]

On Sun, Mar 29, 2020 at 2:22 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Sun, Mar 29, 2020 at 11:16 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > But that magical asm-goto-with-outputs patch probably won't land
> > upstream for a couple of years.
>
> I'm not that familiar with gcc politics, but what's the delay?

No, I mean that _my_ magical patch for the kernel won't land upstream,
simply because even if both gcc and clang supported it today, nobody
would effectively have those compilers for a couple of years..

> ISTM
> having an actual upstream Linux asm-goto-with-outputs that works on
> clang might help light a fire under some butts and/or convince someone
> to pay a gcc developer to implement it on gcc.

Yes, but even for clang, it needs a version that isn't even released yet.

And right now my patch is unconditional. It started out that way
because the whole x86 uaccess.h files were such a mess, and I couldn't
be bothered to fix all the small cases to then have *two* cases (one
for asm goto with outputs, one without).

These days my patch is much simpler (thanks to Al's simplifications),
and I think making it handle both cases would likely not be too
painful.

And in that case I might just commit it, even if effectively nobody
has the clang version installed to make use of it.

Anyway, just in case people want to see it, I'm attaching my current
unconditional patch.

Note that it requires Al's uaccess cleanups, but I do want to point
out how it actually makes for simpler code:

 arch/x86/include/asm/uaccess.h | 72 +++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 40 deletions(-)

and not only does it delete more lines than it adds, the lines it adds
are shorter and simpler than the ones it deletes.

But that "deletes more lines than it adds" is only because it doesn't
even try to build without that "asm goto with outputs" support..

            Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 4489 bytes --]


    Use "asm goto" with outputs for clang testing
---
 arch/x86/include/asm/uaccess.h | 72 +++++++++++++++++++-----------------------
 1 file changed, 32 insertions(+), 40 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index c8247a84244b..9e8b04d4560a 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -279,65 +279,52 @@ do {									\
 } while (0)
 
 #ifdef CONFIG_X86_32
-#define __get_user_asm_u64(x, ptr, retval, errret)			\
+#define __get_user_asm_u64(x, ptr, label)				\
 ({									\
 	__typeof__(ptr) __ptr = (ptr);					\
-	asm volatile("\n"					\
-		     "1:	movl %2,%%eax\n"			\
-		     "2:	movl %3,%%edx\n"			\
-		     "3:\n"				\
-		     ".section .fixup,\"ax\"\n"				\
-		     "4:	mov %4,%0\n"				\
-		     "	xorl %%eax,%%eax\n"				\
-		     "	xorl %%edx,%%edx\n"				\
-		     "	jmp 3b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE_UA(1b, 4b)				\
-		     _ASM_EXTABLE_UA(2b, 4b)				\
-		     : "=r" (retval), "=&A"(x)				\
-		     : "m" (__m(__ptr)), "m" __m(((u32 __user *)(__ptr)) + 1),	\
-		       "i" (errret), "0" (retval));			\
+	asm_volatile_goto("\n"						\
+		     "1:	movl %1,%%eax\n"			\
+		     "2:	movl %2,%%edx\n"			\
+		     _ASM_EXTABLE_UA(1b, %l3)				\
+		     _ASM_EXTABLE_UA(2b, %l3)				\
+		     : "=&A"(x)						\
+		     : "m" (__m(__ptr)), "m" __m(((u32 __user *)(__ptr)) + 1)	\
+		     : : label);					\
 })
 
 #else
-#define __get_user_asm_u64(x, ptr, retval, errret) \
-	 __get_user_asm(x, ptr, retval, "q", "", "=r", errret)
+#define __get_user_asm_u64(x, ptr, label) \
+	 __get_user_asm(x, ptr, "q", "", "=r", label)
 #endif
 
-#define __get_user_size(x, ptr, size, retval, errret)			\
+#define __get_user_size(x, ptr, size, label)				\
 do {									\
-	retval = 0;							\
 	__chk_user_ptr(ptr);						\
 	switch (size) {							\
 	case 1:								\
-		__get_user_asm(x, ptr, retval, "b", "b", "=q", errret);	\
+		__get_user_asm(x, ptr, "b", "b", "=q", label);		\
 		break;							\
 	case 2:								\
-		__get_user_asm(x, ptr, retval, "w", "w", "=r", errret);	\
+		__get_user_asm(x, ptr, "w", "w", "=r", label);		\
 		break;							\
 	case 4:								\
-		__get_user_asm(x, ptr, retval, "l", "k", "=r", errret);	\
+		__get_user_asm(x, ptr, "l", "k", "=r", label);		\
 		break;							\
 	case 8:								\
-		__get_user_asm_u64(x, ptr, retval, errret);		\
+		__get_user_asm_u64(x, ptr, label);			\
 		break;							\
 	default:							\
 		(x) = __get_user_bad();					\
 	}								\
 } while (0)
 
-#define __get_user_asm(x, addr, err, itype, rtype, ltype, errret)	\
-	asm volatile("\n"						\
-		     "1:	mov"itype" %2,%"rtype"1\n"		\
-		     "2:\n"						\
-		     ".section .fixup,\"ax\"\n"				\
-		     "3:	mov %3,%0\n"				\
-		     "	xor"itype" %"rtype"1,%"rtype"1\n"		\
-		     "	jmp 2b\n"					\
-		     ".previous\n"					\
-		     _ASM_EXTABLE_UA(1b, 3b)				\
-		     : "=r" (err), ltype(x)				\
-		     : "m" (__m(addr)), "i" (errret), "0" (err))
+#define __get_user_asm(x, addr, itype, rtype, ltype, label)		\
+	asm_volatile_goto("\n"						\
+		     "1:	mov"itype" %1,%"rtype"0\n"		\
+		     _ASM_EXTABLE_UA(1b, %l2)				\
+		     : ltype(x)						\
+		     : "m" (__m(addr))					\
+		     : : label)
 
 #define __put_user_nocheck(x, ptr, size)			\
 ({								\
@@ -356,14 +343,21 @@ __pu_label:							\
 
 #define __get_user_nocheck(x, ptr, size)				\
 ({									\
+	__label__ __gu_label;						\
 	int __gu_err;							\
 	__inttype(*(ptr)) __gu_val;					\
 	__typeof__(ptr) __gu_ptr = (ptr);				\
 	__typeof__(size) __gu_size = (size);				\
 	__uaccess_begin_nospec();					\
-	__get_user_size(__gu_val, __gu_ptr, __gu_size, __gu_err, -EFAULT);	\
+	__get_user_size(__gu_val, __gu_ptr, __gu_size, __gu_label);	\
 	__uaccess_end();						\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
+	__gu_err = 0;							\
+	if (0) {							\
+__gu_label:								\
+		__uaccess_end();					\
+		__gu_err = -EFAULT;					\
+	}								\
 	__builtin_expect(__gu_err, 0);					\
 })
 
@@ -494,11 +488,9 @@ static __must_check __always_inline bool user_access_begin(const void __user *pt
 
 #define unsafe_get_user(x, ptr, err_label)					\
 do {										\
-	int __gu_err;								\
 	__inttype(*(ptr)) __gu_val;						\
-	__get_user_size(__gu_val, (ptr), sizeof(*(ptr)), __gu_err, -EFAULT);	\
+	__get_user_size(__gu_val, (ptr), sizeof(*(ptr)), err_label);		\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;				\
-	if (unlikely(__gu_err)) goto err_label;					\
 } while (0)
 
 /*