[PATCH 0/4] riscv: uaccess: optimizations

[PATCH 0/4] riscv: uaccess: optimizations

[Jisheng Zhang]

This series tries to optimize riscv uaccess in the following way:

patch1 implement the user_access_begin and families, I.E the unsafe
accessors. when a function like strncpy_from_user() is called,
the userspace access protection is disabled and enabled for every
word read. After patch1, the protection is disabled at the beginning
of the copy and enabled at the end.

patch2 is a simple clean up

patch3 uses 'asm goto' for put_user()
patch4 uses 'asm goto output' for get_user()

Both patch3 and patch4 are based on the fact -- 'asm goto' and
'asm goto output'generates noticeably better code since we don't need
to test the error etc, the exception just jumps to the error handling
directly.


Jisheng Zhang (4):
  riscv: implement user_access_begin and families
  riscv: uaccess: use input constraints for ptr of __put_user
  riscv: uaccess: use 'asm goto' for put_user()
  riscv: uaccess: use 'asm goto output' for get_user

 arch/riscv/include/asm/uaccess.h | 201 +++++++++++++++++++++++--------
 1 file changed, 149 insertions(+), 52 deletions(-)

-- 
2.43.0

[PATCH 1/4] riscv: implement user_access_begin and families

[Jisheng Zhang]

Currently, when a function like strncpy_from_user() is called,
the userspace access protection is disabled and enabled
for every word read.

By implementing user_access_begin and families, the protection
is disabled at the beginning of the copy and enabled at the end.

The __inttype macro is borrowed from x86 implementation.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/include/asm/uaccess.h | 63 ++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 72ec1d9bd3f3..09d4ca37522c 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -28,6 +28,19 @@
 #define __disable_user_access()							\
 	__asm__ __volatile__ ("csrc sstatus, %0" : : "r" (SR_SUM) : "memory")
 
+/*
+ * This is the smallest unsigned integer type that can fit a value
+ * (up to 'long long')
+ */
+#define __inttype(x) __typeof__(		\
+	__typefits(x,char,			\
+	  __typefits(x,short,			\
+	    __typefits(x,int,			\
+	      __typefits(x,long,0ULL)))))
+
+#define __typefits(x,type,not) \
+	__builtin_choose_expr(sizeof(x)<=sizeof(type),(unsigned type)0,not)
+
 /*
  * The exception table consists of pairs of addresses: the first is the
  * address of an instruction that is allowed to fault, and the second is
@@ -335,6 +348,56 @@ do {									\
 		goto err_label;						\
 } while (0)
 
+static __must_check __always_inline bool user_access_begin(const void __user *ptr, size_t len)
+{
+	if (unlikely(!access_ok(ptr,len)))
+		return 0;
+	__enable_user_access();
+	return 1;
+}
+#define user_access_begin(a,b)	user_access_begin(a,b)
+#define user_access_end()	__disable_user_access();
+
+static inline unsigned long user_access_save(void) { return 0UL; }
+static inline void user_access_restore(unsigned long enabled) { }
+
+#define unsafe_put_user(x, ptr, label)	do {				\
+	long __kr_err = 0;						\
+	__put_user_nocheck(x, (ptr), __kr_err);				\
+	if (__kr_err) goto label;					\
+} while (0)
+
+#define unsafe_get_user(x, ptr, label)	do {				\
+	long __kr_err = 0;						\
+	__inttype(*(ptr)) __gu_val;					\
+	__get_user_nocheck(__gu_val, (ptr), __kr_err);			\
+	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
+	if (__kr_err) goto label;					\
+} while (0)
+
+/*
+ * We want the unsafe accessors to always be inlined and use
+ * the error labels - thus the macro games.
+ */
+#define unsafe_copy_loop(dst, src, len, type, label)				\
+	while (len >= sizeof(type)) {						\
+		unsafe_put_user(*(type *)(src),(type __user *)(dst),label);	\
+		dst += sizeof(type);						\
+		src += sizeof(type);						\
+		len -= sizeof(type);						\
+	}
+
+#define unsafe_copy_to_user(_dst,_src,_len,label)			\
+do {									\
+	char __user *__ucu_dst = (_dst);				\
+	const char *__ucu_src = (_src);					\
+	size_t __ucu_len = (_len);					\
+	unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u64, label);	\
+	unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u32, label);	\
+	unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u16, label);	\
+	unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u8, label);	\
+} while (0)
+
 #else /* CONFIG_MMU */
 #include <asm-generic/uaccess.h>
 #endif /* CONFIG_MMU */
-- 
2.43.0

[PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Jisheng Zhang]

I believe the output constraints "=m" is not necessary, because
the instruction itself is "write", we don't need the compiler
to "write" for us. So tell compiler we read from memory instead
of writing.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/include/asm/uaccess.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 09d4ca37522c..84b084e388a7 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -186,11 +186,11 @@ do {								\
 	__typeof__(*(ptr)) __x = x;				\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
-		"	" insn " %z2, %1\n"			\
+		"	" insn " %z1, %2\n"			\
 		"2:\n"						\
 		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0)		\
-		: "+r" (err), "=m" (*(ptr))			\
-		: "rJ" (__x));					\
+		: "+r" (err)			\
+		: "rJ" (__x), "m"(*(ptr)));					\
 } while (0)
 
 #ifdef CONFIG_64BIT
@@ -203,16 +203,16 @@ do {								\
 	u64 __x = (__typeof__((x)-(x)))(x);			\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
-		"	sw %z3, %1\n"				\
+		"	sw %z1, %3\n"				\
 		"2:\n"						\
-		"	sw %z4, %2\n"				\
+		"	sw %z2, %4\n"				\
 		"3:\n"						\
 		_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %0)		\
 		_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %0)		\
-		: "+r" (err),					\
-			"=m" (__ptr[__LSW]),			\
-			"=m" (__ptr[__MSW])			\
-		: "rJ" (__x), "rJ" (__x >> 32));		\
+		: "+r" (err)					\
+		: "rJ" (__x), "rJ" (__x >> 32),			\
+			"m" (__ptr[__LSW]),			\
+			"m" (__ptr[__MSW]));			\
 } while (0)
 #endif /* CONFIG_64BIT */
 
-- 
2.43.0

[PATCH 3/4] riscv: uaccess: use 'asm goto' for put_user()

[Jisheng Zhang]

'asm goto' generates noticeably better code since we don't need to
test the error etc, the exception just jumps to the error handling
directly.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/include/asm/uaccess.h | 68 +++++++++++++++-----------------
 1 file changed, 32 insertions(+), 36 deletions(-)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 84b084e388a7..d8c44705b61d 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -181,61 +181,66 @@ do {								\
 		((x) = (__force __typeof__(x))0, -EFAULT);	\
 })
 
-#define __put_user_asm(insn, x, ptr, err)			\
+#define __put_user_asm(insn, x, ptr, label)			\
 do {								\
 	__typeof__(*(ptr)) __x = x;				\
-	__asm__ __volatile__ (					\
+	asm goto(						\
 		"1:\n"						\
-		"	" insn " %z1, %2\n"			\
-		"2:\n"						\
-		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0)		\
-		: "+r" (err)			\
-		: "rJ" (__x), "m"(*(ptr)));					\
+		"	" insn " %z0, %1\n"			\
+		_ASM_EXTABLE(1b, %l2)				\
+		: : "rJ" (__x), "m"(*(ptr)) : : label);		\
 } while (0)
 
 #ifdef CONFIG_64BIT
-#define __put_user_8(x, ptr, err) \
-	__put_user_asm("sd", x, ptr, err)
+#define __put_user_8(x, ptr, label) \
+	__put_user_asm("sd", x, ptr, label)
 #else /* !CONFIG_64BIT */
-#define __put_user_8(x, ptr, err)				\
+#define __put_user_8(x, ptr, label)				\
 do {								\
 	u32 __user *__ptr = (u32 __user *)(ptr);		\
 	u64 __x = (__typeof__((x)-(x)))(x);			\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
-		"	sw %z1, %3\n"				\
+		"	sw %z0, %2\n"				\
 		"2:\n"						\
-		"	sw %z2, %4\n"				\
-		"3:\n"						\
-		_ASM_EXTABLE_UACCESS_ERR(1b, 3b, %0)		\
-		_ASM_EXTABLE_UACCESS_ERR(2b, 3b, %0)		\
-		: "+r" (err)					\
-		: "rJ" (__x), "rJ" (__x >> 32),			\
+		"	sw %z1, %3\n"				\
+		_ASM_EXTABLE(1b, %l4)				\
+		_ASM_EXTABLE(2b, %l4)				\
+		: : "rJ" (__x), "rJ" (__x >> 32),		\
 			"m" (__ptr[__LSW]),			\
-			"m" (__ptr[__MSW]));			\
+			"m" (__ptr[__MSW]) : : label);		\
 } while (0)
 #endif /* CONFIG_64BIT */
 
-#define __put_user_nocheck(x, __gu_ptr, __pu_err)					\
+#define __put_user_nocheck(x, __gu_ptr, label)			\
 do {								\
 	switch (sizeof(*__gu_ptr)) {				\
 	case 1:							\
-		__put_user_asm("sb", (x), __gu_ptr, __pu_err);	\
+		__put_user_asm("sb", (x), __gu_ptr, label);	\
 		break;						\
 	case 2:							\
-		__put_user_asm("sh", (x), __gu_ptr, __pu_err);	\
+		__put_user_asm("sh", (x), __gu_ptr, label);	\
 		break;						\
 	case 4:							\
-		__put_user_asm("sw", (x), __gu_ptr, __pu_err);	\
+		__put_user_asm("sw", (x), __gu_ptr, label);	\
 		break;						\
 	case 8:							\
-		__put_user_8((x), __gu_ptr, __pu_err);	\
+		__put_user_8((x), __gu_ptr, label);		\
 		break;						\
 	default:						\
 		BUILD_BUG();					\
 	}							\
 } while (0)
 
+#define __put_user_error(x, ptr, err)				\
+do {								\
+	__label__ err_label;					\
+	__put_user_nocheck(x, ptr, err_label);			\
+	break;							\
+err_label:							\
+	(err) = -EFAULT;					\
+} while (0)
+
 /**
  * __put_user: - Write a simple value into user space, with less checking.
  * @x:   Value to copy to user space.
@@ -266,7 +271,7 @@ do {								\
 	__chk_user_ptr(__gu_ptr);				\
 								\
 	__enable_user_access();					\
-	__put_user_nocheck(__val, __gu_ptr, __pu_err);		\
+	__put_user_error(__val, __gu_ptr, __pu_err);		\
 	__disable_user_access();				\
 								\
 	__pu_err;						\
@@ -340,13 +345,7 @@ do {									\
 } while (0)
 
 #define __put_kernel_nofault(dst, src, type, err_label)			\
-do {									\
-	long __kr_err = 0;						\
-									\
-	__put_user_nocheck(*((type *)(src)), (type *)(dst), __kr_err);	\
-	if (unlikely(__kr_err))						\
-		goto err_label;						\
-} while (0)
+	__put_user_nocheck(*((type *)(src)), (type *)(dst), err_label);
 
 static __must_check __always_inline bool user_access_begin(const void __user *ptr, size_t len)
 {
@@ -361,11 +360,8 @@ static __must_check __always_inline bool user_access_begin(const void __user *pt
 static inline unsigned long user_access_save(void) { return 0UL; }
 static inline void user_access_restore(unsigned long enabled) { }
 
-#define unsafe_put_user(x, ptr, label)	do {				\
-	long __kr_err = 0;						\
-	__put_user_nocheck(x, (ptr), __kr_err);				\
-	if (__kr_err) goto label;					\
-} while (0)
+#define unsafe_put_user(x, ptr, label)					\
+	__put_user_nocheck(x, (ptr), label);
 
 #define unsafe_get_user(x, ptr, label)	do {				\
 	long __kr_err = 0;						\
-- 
2.43.0

[PATCH 4/4] riscv: uaccess: use 'asm goto output' for get_user

[Jisheng Zhang]

'asm goto output' generates noticeably better code since we don't need
to test the error etc, the exception just jumps to the error handling
directly.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/include/asm/uaccess.h | 88 +++++++++++++++++++++++---------
 1 file changed, 63 insertions(+), 25 deletions(-)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index d8c44705b61d..d9c32b4f7d13 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -63,27 +63,54 @@
  * call.
  */
 
-#define __get_user_asm(insn, x, ptr, err)			\
+#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
+#define __get_user_asm(insn, x, ptr, label)			\
+	asm_goto_output(					\
+		"1:\n"						\
+		"	" insn " %0, %1\n"			\
+		_ASM_EXTABLE_UACCESS_ERR(1b, %l2, %0)		\
+		: "=&r" (x)					\
+		: "m" (*(ptr)) : : label)
+#else /* !CONFIG_CC_HAS_ASM_GOTO_OUTPUT */
+#define __get_user_asm(insn, x, ptr, label)			\
 do {								\
-	__typeof__(x) __x;					\
+	long __gua_err = 0;					\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
 		"	" insn " %1, %2\n"			\
 		"2:\n"						\
 		_ASM_EXTABLE_UACCESS_ERR_ZERO(1b, 2b, %0, %1)	\
-		: "+r" (err), "=&r" (__x)			\
+		: "+r" (__gua_err), "=&r" (x)			\
 		: "m" (*(ptr)));				\
-	(x) = __x;						\
+	if (__gua_err)						\
+		goto label;					\
 } while (0)
+#endif /* CONFIG_CC_HAS_ASM_GOTO_OUTPUT */
 
 #ifdef CONFIG_64BIT
-#define __get_user_8(x, ptr, err) \
-	__get_user_asm("ld", x, ptr, err)
+#define __get_user_8(x, ptr, label) \
+	__get_user_asm("ld", x, ptr, label)
 #else /* !CONFIG_64BIT */
-#define __get_user_8(x, ptr, err)				\
+
+#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
+#define __get_user_8(x, ptr, label)				\
+	asm_goto_output(					\
+		"1:\n"						\
+		"	lw %0, %2\n"				\
+		"2:\n"						\
+		"	lw %1, %3\n"				\
+		_ASM_EXTABLE_UACCESS_ERR(1b, %l4, %0)		\
+		_ASM_EXTABLE_UACCESS_ERR(2b, %l4, %0)		\
+		: "=&r" (__lo), "=r" (__hi)			\
+		: "m" (__ptr[__LSW]), "m" (__ptr[__MSW])	\
+		: : label)
+
+#else /* !CONFIG_CC_HAS_ASM_GOTO_OUTPUT */
+#define __get_user_8(x, ptr, label)				\
 do {								\
 	u32 __user *__ptr = (u32 __user *)(ptr);		\
 	u32 __lo, __hi;						\
+	long __gu8_err = 0;					\
 	__asm__ __volatile__ (					\
 		"1:\n"						\
 		"	lw %1, %3\n"				\
@@ -92,35 +119,51 @@ do {								\
 		"3:\n"						\
 		_ASM_EXTABLE_UACCESS_ERR_ZERO(1b, 3b, %0, %1)	\
 		_ASM_EXTABLE_UACCESS_ERR_ZERO(2b, 3b, %0, %1)	\
-		: "+r" (err), "=&r" (__lo), "=r" (__hi)		\
+		: "+r" (__gu8_err), "=&r" (__lo), "=r" (__hi)	\
 		: "m" (__ptr[__LSW]), "m" (__ptr[__MSW]));	\
-	if (err)						\
+	if (__gu8_err) {					\
 		__hi = 0;					\
+		goto label;					\
+	}							\
 	(x) = (__typeof__(x))((__typeof__((x)-(x)))(		\
 		(((u64)__hi << 32) | __lo)));			\
 } while (0)
+#endif /* CONFIG_CC_HAS_ASM_GOTO_OUTPUT */
+
 #endif /* CONFIG_64BIT */
 
-#define __get_user_nocheck(x, __gu_ptr, __gu_err)		\
+#define __get_user_nocheck(x, __gu_ptr, label)			\
 do {								\
 	switch (sizeof(*__gu_ptr)) {				\
 	case 1:							\
-		__get_user_asm("lb", (x), __gu_ptr, __gu_err);	\
+		__get_user_asm("lb", (x), __gu_ptr, label);	\
 		break;						\
 	case 2:							\
-		__get_user_asm("lh", (x), __gu_ptr, __gu_err);	\
+		__get_user_asm("lh", (x), __gu_ptr, label);	\
 		break;						\
 	case 4:							\
-		__get_user_asm("lw", (x), __gu_ptr, __gu_err);	\
+		__get_user_asm("lw", (x), __gu_ptr, label);	\
 		break;						\
 	case 8:							\
-		__get_user_8((x), __gu_ptr, __gu_err);	\
+		__get_user_8((x), __gu_ptr, label);		\
 		break;						\
 	default:						\
 		BUILD_BUG();					\
 	}							\
 } while (0)
 
+#define __get_user_error(x, ptr, err)					\
+do {									\
+	__label__ __gu_failed;						\
+									\
+	__get_user_nocheck(x, ptr, __gu_failed);			\
+		err = 0;						\
+		break;							\
+__gu_failed:								\
+		x = 0;							\
+		err = -EFAULT;						\
+} while (0)
+
 /**
  * __get_user: - Get a simple variable from user space, with less checking.
  * @x:   Variable to store result.
@@ -145,13 +188,16 @@ do {								\
 ({								\
 	const __typeof__(*(ptr)) __user *__gu_ptr = (ptr);	\
 	long __gu_err = 0;					\
+	__typeof__(x) __gu_val;					\
 								\
 	__chk_user_ptr(__gu_ptr);				\
 								\
 	__enable_user_access();					\
-	__get_user_nocheck(x, __gu_ptr, __gu_err);		\
+	__get_user_error(__gu_val, __gu_ptr, __gu_err);		\
 	__disable_user_access();				\
 								\
+	(x) = __gu_val;						\
+								\
 	__gu_err;						\
 })
 
@@ -336,13 +382,7 @@ unsigned long __must_check clear_user(void __user *to, unsigned long n)
 }
 
 #define __get_kernel_nofault(dst, src, type, err_label)			\
-do {									\
-	long __kr_err = 0;						\
-									\
-	__get_user_nocheck(*((type *)(dst)), (type *)(src), __kr_err);	\
-	if (unlikely(__kr_err))						\
-		goto err_label;						\
-} while (0)
+	__get_user_nocheck(*((type *)(dst)), (type *)(src), err_label);
 
 #define __put_kernel_nofault(dst, src, type, err_label)			\
 	__put_user_nocheck(*((type *)(src)), (type *)(dst), err_label);
@@ -364,11 +404,9 @@ static inline void user_access_restore(unsigned long enabled) { }
 	__put_user_nocheck(x, (ptr), label);
 
 #define unsafe_get_user(x, ptr, label)	do {				\
-	long __kr_err = 0;						\
 	__inttype(*(ptr)) __gu_val;					\
-	__get_user_nocheck(__gu_val, (ptr), __kr_err);			\
+	__get_user_nocheck(__gu_val, (ptr), label);			\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
-	if (__kr_err) goto label;					\
 } while (0)
 
 /*
-- 
2.43.0

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Arnd Bergmann]

On Tue, Jun 25, 2024, at 06:04, Jisheng Zhang wrote:
> I believe the output constraints "=m" is not necessary, because
> the instruction itself is "write", we don't need the compiler
> to "write" for us. So tell compiler we read from memory instead
> of writing.
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>

I think this is a bit too confusing: clearly there is no
read access from the __user pointer, so what you add in here
is not correct. There also needs to be a code comment about
why you do it this way, as it's not clear that this is
a workaround for old compilers without
CONFIG_CC_HAS_ASM_GOTO_OUTPUT.

> index 09d4ca37522c..84b084e388a7 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -186,11 +186,11 @@ do {								\
>  	__typeof__(*(ptr)) __x = x;				\
>  	__asm__ __volatile__ (					\
>  		"1:\n"						\
> -		"	" insn " %z2, %1\n"			\
> +		"	" insn " %z1, %2\n"			\
>  		"2:\n"						\
>  		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0)		\
> -		: "+r" (err), "=m" (*(ptr))			\
> -		: "rJ" (__x));					\
> +		: "+r" (err)			\
> +		: "rJ" (__x), "m"(*(ptr)));					\
>  } while (0)
> 

I suspect this could just be a "r" constraint instead of
"m", treating the __user pointer as a plain integer.

For kernel pointers, using "m" and "=m" constraints
correctly is necessary since gcc will often access the
same data from C code as well. For __user pointers, we
can probably get away without it since no C code is
ever allowed to just dereference them. If you do that,
you may want to have the same thing in the __get_user
side.

      Arnd

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Arnd Bergmann]

On Tue, Jun 25, 2024, at 06:04, Jisheng Zhang wrote:
> This series tries to optimize riscv uaccess in the following way:
>
> patch1 implement the user_access_begin and families, I.E the unsafe
> accessors. when a function like strncpy_from_user() is called,
> the userspace access protection is disabled and enabled for every
> word read. After patch1, the protection is disabled at the beginning
> of the copy and enabled at the end.
>
> patch2 is a simple clean up
>
> patch3 uses 'asm goto' for put_user()
> patch4 uses 'asm goto output' for get_user()
>
> Both patch3 and patch4 are based on the fact -- 'asm goto' and
> 'asm goto output'generates noticeably better code since we don't need
> to test the error etc, the exception just jumps to the error handling
> directly.

Nice!

I hope that we can one day make this more straightforward
and share more of the implementation across architectures,
in particular the bits that are just wrappers around
the low-level asm.

We have something like this already on powerpc and x86,
and Linus just did the version for arm64, which I assume
you are using as a template for this. Four architectures
is probably the point at which we should try to consolidate
the code again and move as much as we can into asm-generic.

I think the only bets we really need from an architecture
here are:

- __enable_user_access()/__disable_user_access() in
   the label version
- __raw_get_mem_{1,2,4,8}() and __raw_put_mem_{1,2,4,8}()

and then we can build all the normal interface functions
on those in a way that assumes we have as goto available,
with the appropriate fallback in __raw_get_mem_*() as
long as we need to support gcc-10 and older.

      Arnd

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Linus Torvalds]

On Tue, 25 Jun 2024 at 00:22, Arnd Bergmann <arnd@arndb.de> wrote:
>
> I think the only bets we really need from an architecture
> here are:
>
> - __enable_user_access()/__disable_user_access() in
>    the label version

KCSAN wants user_access_save/restore() too, but yes.

> - __raw_get_mem_{1,2,4,8}() and __raw_put_mem_{1,2,4,8}()

You still need to split it into user/kernel.

Yes, *often* there is just one address space and they can be one "mem"
accessor thing, but we do have architectures with actually separate
user and kernel address spaces.

But yes, it would be lovely if we did things as "implement the
low-level accessor functions and we'll wrap them for the generic case"
rather than have every architecture have to do the wrapping..

The wrapping isn't just the label-based "unsafe" accesses and the
traditional error number return case, it's also the size-based case
statements ("poor man's generics").

The problem, of course, is that the majority of architectures are
based on the legacy interfaces, so it's a lot of annoying low-level
small details. I think there's a reason why nobody did the arm64
"unsafe" ones - the patch didn't end up being that bad, but it's just
fairly grotty code.

But apparently the arm64 patch was simple enough that at least RISC-V
people went "that doesn't look so bad".

Maybe other architectures will also be fairly straightforward when
there's a fairly clear example of how it should be done.

             Linus

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Jisheng Zhang]

On Tue, Jun 25, 2024 at 07:54:30AM +0200, Arnd Bergmann wrote:
> On Tue, Jun 25, 2024, at 06:04, Jisheng Zhang wrote:
> > I believe the output constraints "=m" is not necessary, because
> > the instruction itself is "write", we don't need the compiler
> > to "write" for us. So tell compiler we read from memory instead
> > of writing.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> 
> I think this is a bit too confusing: clearly there is no
> read access from the __user pointer, so what you add in here
> is not correct. There also needs to be a code comment about

Here is my understanding: the __put_user is implemented with
sd(or its less wider variant, sw etc.), w/o considering the
ex_table, the previous code can be simplified as below:

__asm__ __volatile__ (
	"sw	%z2, %1\n"
	: "+r" (err), "=m" (*(ptr))
	: "rJ" (__x));

Here ptr is really an input, just tells gcc where to store,
And the "store" action is from the "sw" instruction, I don't
need the gcc generates "store" instruction for me. so IMHO,
there's no need to use output constraints here. so I changed
it to

__asm__ __volatile__ (
	"sw	%z1, %2\n"
	: "+r" (err)
	: "rJ" (__x), "m"(*(ptr)));

The key here: is this correct?


Here is the put_user piece code and comments from x86

/*
 * Tell gcc we read from memory instead of writing: this is because
 * we do not write to any memory gcc knows about, so there are no
 * aliasing issues.
 */
#define __put_user_goto(x, addr, itype, ltype, label)                   \
        asm goto("\n"                                                   \
                "1:     mov"itype" %0,%1\n"                             \
                _ASM_EXTABLE_UA(1b, %l2)                                \
                : : ltype(x), "m" (__m(addr))                           \
                : : label)


As can be seen, x86 also doesn't put the (addr) in output constraints,
I think x86 version did similar modification in history, but when I tried
to searh the git history, the comment is there from the git first day.

Any hint or suggestion is appreciated!

> why you do it this way, as it's not clear that this is
> a workaround for old compilers without
> CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
> 
> > index 09d4ca37522c..84b084e388a7 100644
> > --- a/arch/riscv/include/asm/uaccess.h
> > +++ b/arch/riscv/include/asm/uaccess.h
> > @@ -186,11 +186,11 @@ do {								\
> >  	__typeof__(*(ptr)) __x = x;				\
> >  	__asm__ __volatile__ (					\
> >  		"1:\n"						\
> > -		"	" insn " %z2, %1\n"			\
> > +		"	" insn " %z1, %2\n"			\
> >  		"2:\n"						\
> >  		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0)		\
> > -		: "+r" (err), "=m" (*(ptr))			\
> > -		: "rJ" (__x));					\
> > +		: "+r" (err)			\
> > +		: "rJ" (__x), "m"(*(ptr)));					\
> >  } while (0)
> > 
> 
> I suspect this could just be a "r" constraint instead of
> "m", treating the __user pointer as a plain integer.

I tried "r", the generated code is not as good as "m"

for example
__put_user(0x12, &frame->uc.uc_flags);

with "m", the generated code will be

...
csrs    sstatus,a5
li      a4,18
sd      a4,128(s1)
csrc    sstatus,a5
...


with "r", the generated code will be

...
csrs    sstatus,a5
li      a4,18
addi    s1,s1,128
sd      a4,0(s1)
csrc    sstatus,a5
...

As can be seen, "m" can make use of the 'offset' of
sd, so save one instruction.

> 
> For kernel pointers, using "m" and "=m" constraints
> correctly is necessary since gcc will often access the
> same data from C code as well. For __user pointers, we
> can probably get away without it since no C code is
> ever allowed to just dereference them. If you do that,
> you may want to have the same thing in the __get_user
> side.
> 
>       Arnd

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Jisheng Zhang]

On Wed, Jun 26, 2024 at 08:32:38PM +0800, Jisheng Zhang wrote:
> On Tue, Jun 25, 2024 at 07:54:30AM +0200, Arnd Bergmann wrote:
> > On Tue, Jun 25, 2024, at 06:04, Jisheng Zhang wrote:
> > > I believe the output constraints "=m" is not necessary, because
> > > the instruction itself is "write", we don't need the compiler
> > > to "write" for us. So tell compiler we read from memory instead
> > > of writing.
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > 
> > I think this is a bit too confusing: clearly there is no
> > read access from the __user pointer, so what you add in here
> > is not correct. There also needs to be a code comment about
> 
> Here is my understanding: the __put_user is implemented with
> sd(or its less wider variant, sw etc.), w/o considering the
> ex_table, the previous code can be simplified as below:
> 
> __asm__ __volatile__ (
> 	"sw	%z2, %1\n"
> 	: "+r" (err), "=m" (*(ptr))
> 	: "rJ" (__x));
> 
> Here ptr is really an input, just tells gcc where to store,
> And the "store" action is from the "sw" instruction, I don't
> need the gcc generates "store" instruction for me. so IMHO,
> there's no need to use output constraints here. so I changed
> it to
> 
> __asm__ __volatile__ (
> 	"sw	%z1, %2\n"
> 	: "+r" (err)
> 	: "rJ" (__x), "m"(*(ptr)));
> 
> The key here: is this correct?
> 
> 
> Here is the put_user piece code and comments from x86
> 
> /*
>  * Tell gcc we read from memory instead of writing: this is because
>  * we do not write to any memory gcc knows about, so there are no
>  * aliasing issues.
>  */
> #define __put_user_goto(x, addr, itype, ltype, label)                   \
>         asm goto("\n"                                                   \
>                 "1:     mov"itype" %0,%1\n"                             \
>                 _ASM_EXTABLE_UA(1b, %l2)                                \
>                 : : ltype(x), "m" (__m(addr))                           \
>                 : : label)

Here is the simplified put_user piece code of arm64:

#define __put_mem_asm(store, reg, x, addr, err, type)                   \
        asm volatile(                                                   \
        "1:     " store "       " reg "1, [%2]\n"                       \
        "2:\n"                                                          \
        _ASM_EXTABLE_##type##ACCESS_ERR(1b, 2b, %w0)                    \
        : "+r" (err)                                                    \
        : "rZ" (x), "r" (addr))

no output constraints either. It just uses "r" input constraints to tell
gcc to read the store address into one proper GP reg.

> 
> 
> As can be seen, x86 also doesn't put the (addr) in output constraints,
> I think x86 version did similar modification in history, but when I tried
> to searh the git history, the comment is there from the git first day.
> 
> Any hint or suggestion is appreciated!
> 
> > why you do it this way, as it's not clear that this is
> > a workaround for old compilers without
> > CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
> > 
> > > index 09d4ca37522c..84b084e388a7 100644
> > > --- a/arch/riscv/include/asm/uaccess.h
> > > +++ b/arch/riscv/include/asm/uaccess.h
> > > @@ -186,11 +186,11 @@ do {								\
> > >  	__typeof__(*(ptr)) __x = x;				\
> > >  	__asm__ __volatile__ (					\
> > >  		"1:\n"						\
> > > -		"	" insn " %z2, %1\n"			\
> > > +		"	" insn " %z1, %2\n"			\
> > >  		"2:\n"						\
> > >  		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0)		\
> > > -		: "+r" (err), "=m" (*(ptr))			\
> > > -		: "rJ" (__x));					\
> > > +		: "+r" (err)			\
> > > +		: "rJ" (__x), "m"(*(ptr)));					\
> > >  } while (0)
> > > 
> > 
> > I suspect this could just be a "r" constraint instead of
> > "m", treating the __user pointer as a plain integer.
> 
> I tried "r", the generated code is not as good as "m"
> 
> for example
> __put_user(0x12, &frame->uc.uc_flags);
> 
> with "m", the generated code will be
> 
> ...
> csrs    sstatus,a5
> li      a4,18
> sd      a4,128(s1)
> csrc    sstatus,a5
> ...
> 
> 
> with "r", the generated code will be
> 
> ...
> csrs    sstatus,a5
> li      a4,18
> addi    s1,s1,128
> sd      a4,0(s1)
> csrc    sstatus,a5
> ...
> 
> As can be seen, "m" can make use of the 'offset' of
> sd, so save one instruction.
> 
> > 
> > For kernel pointers, using "m" and "=m" constraints
> > correctly is necessary since gcc will often access the
> > same data from C code as well. For __user pointers, we
> > can probably get away without it since no C code is
> > ever allowed to just dereference them. If you do that,
> > you may want to have the same thing in the __get_user
> > side.
> > 
> >       Arnd

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Jisheng Zhang]

On Tue, Jun 25, 2024 at 11:12:16AM -0700, Linus Torvalds wrote:
> On Tue, 25 Jun 2024 at 00:22, Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > I think the only bets we really need from an architecture
> > here are:
> >
> > - __enable_user_access()/__disable_user_access() in
> >    the label version
> 
> KCSAN wants user_access_save/restore() too, but yes.
> 
> > - __raw_get_mem_{1,2,4,8}() and __raw_put_mem_{1,2,4,8}()
> 
> You still need to split it into user/kernel.
> 
> Yes, *often* there is just one address space and they can be one "mem"
> accessor thing, but we do have architectures with actually separate
> user and kernel address spaces.
> 
> But yes, it would be lovely if we did things as "implement the
> low-level accessor functions and we'll wrap them for the generic case"
> rather than have every architecture have to do the wrapping..
> 
> The wrapping isn't just the label-based "unsafe" accesses and the
> traditional error number return case, it's also the size-based case
> statements ("poor man's generics").
> 
> The problem, of course, is that the majority of architectures are
> based on the legacy interfaces, so it's a lot of annoying low-level
> small details. I think there's a reason why nobody did the arm64
> "unsafe" ones - the patch didn't end up being that bad, but it's just
> fairly grotty code.
> 
> But apparently the arm64 patch was simple enough that at least RISC-V
> people went "that doesn't look so bad".
> 
> Maybe other architectures will also be fairly straightforward when
> there's a fairly clear example of how it should be done.

>> We have something like this already on powerpc and x86,
>> and Linus just did the version for arm64, which I assume
>> you are using as a template for this. Four architectures

Yep, this series is inspired by Linus's patch series, and to be honest,
some code is borrowed from his arm64 version ;) I saw he improved
arm64, then I thought a nice improvement we want for riscv too, can I do
similarly for riscv? 

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Jisheng Zhang]

On Wed, Jun 26, 2024 at 03:12:50PM +0200, Andreas Schwab wrote:
> On Jun 25 2024, Jisheng Zhang wrote:
> 
> > I believe the output constraints "=m" is not necessary, because
> > the instruction itself is "write", we don't need the compiler
> > to "write" for us.
> 
> No, this is backwards.  Being an output operand means that the *asm* is
> writing to it, and the compiler can read the value from there afterwards
> (and the previous value is dead before the asm).

Hi Andreas,

I compared tens of __put_user() caller's generated code between orig
version and patched version, they are the same. Sure maybe this is
not enough. 

But your explanation can be applied to x86 and arm64 __put_user()
implementations, asm is also writing, then why there's no output
constraints there?(see the other two emails)? Could you please help
me to understand the tricky points?

Thanks in advance

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Andreas Schwab]

On Jun 25 2024, Jisheng Zhang wrote:

> I believe the output constraints "=m" is not necessary, because
> the instruction itself is "write", we don't need the compiler
> to "write" for us.

No, this is backwards.  Being an output operand means that the *asm* is
writing to it, and the compiler can read the value from there afterwards
(and the previous value is dead before the asm).

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Jisheng Zhang]

On Wed, Jun 26, 2024 at 08:49:59PM +0800, Jisheng Zhang wrote:
> On Wed, Jun 26, 2024 at 08:32:38PM +0800, Jisheng Zhang wrote:
> > On Tue, Jun 25, 2024 at 07:54:30AM +0200, Arnd Bergmann wrote:
> > > On Tue, Jun 25, 2024, at 06:04, Jisheng Zhang wrote:
> > > > I believe the output constraints "=m" is not necessary, because
> > > > the instruction itself is "write", we don't need the compiler
> > > > to "write" for us. So tell compiler we read from memory instead
> > > > of writing.
> > > >
> > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > 
> > > I think this is a bit too confusing: clearly there is no
> > > read access from the __user pointer, so what you add in here
> > > is not correct. There also needs to be a code comment about
> > 
> > Here is my understanding: the __put_user is implemented with
> > sd(or its less wider variant, sw etc.), w/o considering the
> > ex_table, the previous code can be simplified as below:
> > 
> > __asm__ __volatile__ (
> > 	"sw	%z2, %1\n"
> > 	: "+r" (err), "=m" (*(ptr))
> > 	: "rJ" (__x));
> > 
> > Here ptr is really an input, just tells gcc where to store,
> > And the "store" action is from the "sw" instruction, I don't
> > need the gcc generates "store" instruction for me. so IMHO,
> > there's no need to use output constraints here. so I changed
> > it to
> > 
> > __asm__ __volatile__ (
> > 	"sw	%z1, %2\n"
> > 	: "+r" (err)
> > 	: "rJ" (__x), "m"(*(ptr)));
> > 
> > The key here: is this correct?
> > 
> > 
> > Here is the put_user piece code and comments from x86
> > 
> > /*
> >  * Tell gcc we read from memory instead of writing: this is because
> >  * we do not write to any memory gcc knows about, so there are no
> >  * aliasing issues.
> >  */
> > #define __put_user_goto(x, addr, itype, ltype, label)                   \
> >         asm goto("\n"                                                   \
> >                 "1:     mov"itype" %0,%1\n"                             \
> >                 _ASM_EXTABLE_UA(1b, %l2)                                \
> >                 : : ltype(x), "m" (__m(addr))                           \
> >                 : : label)
> 
> Here is the simplified put_user piece code of arm64:
> 
> #define __put_mem_asm(store, reg, x, addr, err, type)                   \
>         asm volatile(                                                   \
>         "1:     " store "       " reg "1, [%2]\n"                       \
>         "2:\n"                                                          \
>         _ASM_EXTABLE_##type##ACCESS_ERR(1b, 2b, %w0)                    \
>         : "+r" (err)                                                    \
>         : "rZ" (x), "r" (addr))
> 
> no output constraints either. It just uses "r" input constraints to tell

make it accurate: by this I mean the "addr" of __put_user() isn't
in the output constraints.

> gcc to read the store address into one proper GP reg.
> 
> > 
> > 
> > As can be seen, x86 also doesn't put the (addr) in output constraints,
> > I think x86 version did similar modification in history, but when I tried
> > to searh the git history, the comment is there from the git first day.
> > 
> > Any hint or suggestion is appreciated!
> > 
> > > why you do it this way, as it's not clear that this is
> > > a workaround for old compilers without
> > > CONFIG_CC_HAS_ASM_GOTO_OUTPUT.
> > > 
> > > > index 09d4ca37522c..84b084e388a7 100644
> > > > --- a/arch/riscv/include/asm/uaccess.h
> > > > +++ b/arch/riscv/include/asm/uaccess.h
> > > > @@ -186,11 +186,11 @@ do {								\
> > > >  	__typeof__(*(ptr)) __x = x;				\
> > > >  	__asm__ __volatile__ (					\
> > > >  		"1:\n"						\
> > > > -		"	" insn " %z2, %1\n"			\
> > > > +		"	" insn " %z1, %2\n"			\
> > > >  		"2:\n"						\
> > > >  		_ASM_EXTABLE_UACCESS_ERR(1b, 2b, %0)		\
> > > > -		: "+r" (err), "=m" (*(ptr))			\
> > > > -		: "rJ" (__x));					\
> > > > +		: "+r" (err)			\
> > > > +		: "rJ" (__x), "m"(*(ptr)));					\
> > > >  } while (0)
> > > > 
> > > 
> > > I suspect this could just be a "r" constraint instead of
> > > "m", treating the __user pointer as a plain integer.
> > 
> > I tried "r", the generated code is not as good as "m"
> > 
> > for example
> > __put_user(0x12, &frame->uc.uc_flags);
> > 
> > with "m", the generated code will be
> > 
> > ...
> > csrs    sstatus,a5
> > li      a4,18
> > sd      a4,128(s1)
> > csrc    sstatus,a5
> > ...
> > 
> > 
> > with "r", the generated code will be
> > 
> > ...
> > csrs    sstatus,a5
> > li      a4,18
> > addi    s1,s1,128
> > sd      a4,0(s1)
> > csrc    sstatus,a5
> > ...
> > 
> > As can be seen, "m" can make use of the 'offset' of
> > sd, so save one instruction.
> > 
> > > 
> > > For kernel pointers, using "m" and "=m" constraints
> > > correctly is necessary since gcc will often access the
> > > same data from C code as well. For __user pointers, we
> > > can probably get away without it since no C code is
> > > ever allowed to just dereference them. If you do that,
> > > you may want to have the same thing in the __get_user
> > > side.
> > > 
> > >       Arnd

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Andreas Schwab]

On Jun 26 2024, Jisheng Zhang wrote:

> no output constraints either. It just uses "r" input constraints to tell
> gcc to read the store address into one proper GP reg.

Again, this is backwards.  Being an input operand means the asm is using
this operand as an input to the instructions.  The compiler needs to
arrange to put the value in the allocated operand location according to
the constraint.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Jisheng Zhang]

On Wed, Jun 26, 2024 at 03:35:54PM +0200, Andreas Schwab wrote:
> On Jun 26 2024, Jisheng Zhang wrote:
> 
> > no output constraints either. It just uses "r" input constraints to tell
> > gcc to read the store address into one proper GP reg.
> 
> Again, this is backwards.  Being an input operand means the asm is using
> this operand as an input to the instructions.  The compiler needs to
> arrange to put the value in the allocated operand location according to
> the constraint.

Hi Andreas,

Your information is clearly received. What confused me is:

why x86 and arm64 don't put the "addr" of __put_user into output
constraints? Especially the following comments, why this is "read"
from memory?

 * Tell gcc we read from memory instead of writing: this is because
 * we do not write to any memory gcc knows about, so there are no
 * aliasing issues.

can you please kindly help me understand the tricky points here?

thanks
> 
> -- 
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Arnd Bergmann]

On Wed, Jun 26, 2024, at 15:12, Jisheng Zhang wrote:
> On Wed, Jun 26, 2024 at 03:12:50PM +0200, Andreas Schwab wrote:
>> On Jun 25 2024, Jisheng Zhang wrote:
>> 
>> > I believe the output constraints "=m" is not necessary, because
>> > the instruction itself is "write", we don't need the compiler
>> > to "write" for us.
>> 
>> No, this is backwards.  Being an output operand means that the *asm* is
>> writing to it, and the compiler can read the value from there afterwards
>> (and the previous value is dead before the asm).
>
> Hi Andreas,
>
> I compared tens of __put_user() caller's generated code between orig
> version and patched version, they are the same. Sure maybe this is
> not enough. 
>
> But your explanation can be applied to x86 and arm64 __put_user()
> implementations, asm is also writing, then why there's no output
> constraints there?(see the other two emails)? Could you please help
> me to understand the tricky points?

I think part of the reason for the specific way the x86
user access is written is to work around bugs in old
compiler versions, as well as to take advantage of the
complex addressing modes in x86 assembler, see this bit
that dates back to the earliest version of the x86_64
codebase and is still left in place:

/* FIXME: this hack is definitely wrong -AK */
struct __large_struct { unsigned long buf[100]; };
#define __m(x) (*(struct __large_struct __user *)(x))

Using the memory input constraint means that x86 can use
a load from a pointer plus offset, but riscv doesn't
actually do this. The __large_struct I think was needed
either to prevent the compiler from reading the data
outside of the assembly, or to tell the compiler about
the fact that there is an actual memory access if
__put_user() was pointed at kernel memory.

If you just copy from the arm64 version that uses an
"r"(address) constraint instead of the "m"(*address)
version, it should be fine for any user space access.

The output constraint is technically still be needed
if we have code like this one where we actually write to
something in kernel space:

int f(void)
{
     int a = 1;
     int b = 2;
     __put_kernel_nofault(&a, &b, int, error);
     return a;
error:
     return -EFAULT;
}

In this case, __put_kernel_nofault() writes the value
of b into a, but the compiler can safely assume that
a is not changed by the assembly because there is no
memory output, and would likely just return a constant '1'. 

For put_user(), this cannot happen because the compiler
doesn't know anything about the contents of the __user
pointer. For __put_kernel_nofault(), we rely on the
callers never using it on pointers they access, which
is probably a reasonable assumption, but not entirely
correct.

     Arnd

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Jisheng Zhang]

On Wed, Jun 26, 2024 at 04:25:26PM +0200, Arnd Bergmann wrote:
> On Wed, Jun 26, 2024, at 15:12, Jisheng Zhang wrote:
> > On Wed, Jun 26, 2024 at 03:12:50PM +0200, Andreas Schwab wrote:
> >> On Jun 25 2024, Jisheng Zhang wrote:
> >> 
> >> > I believe the output constraints "=m" is not necessary, because
> >> > the instruction itself is "write", we don't need the compiler
> >> > to "write" for us.
> >> 
> >> No, this is backwards.  Being an output operand means that the *asm* is
> >> writing to it, and the compiler can read the value from there afterwards
> >> (and the previous value is dead before the asm).
> >
> > Hi Andreas,
> >
> > I compared tens of __put_user() caller's generated code between orig
> > version and patched version, they are the same. Sure maybe this is
> > not enough. 
> >
> > But your explanation can be applied to x86 and arm64 __put_user()
> > implementations, asm is also writing, then why there's no output
> > constraints there?(see the other two emails)? Could you please help
> > me to understand the tricky points?
> 
> I think part of the reason for the specific way the x86
> user access is written is to work around bugs in old
> compiler versions, as well as to take advantage of the
> complex addressing modes in x86 assembler, see this bit
> that dates back to the earliest version of the x86_64
> codebase and is still left in place:
> 
> /* FIXME: this hack is definitely wrong -AK */
> struct __large_struct { unsigned long buf[100]; };
> #define __m(x) (*(struct __large_struct __user *)(x))
> 
> Using the memory input constraint means that x86 can use
> a load from a pointer plus offset, but riscv doesn't
> actually do this. The __large_struct I think was needed
> either to prevent the compiler from reading the data
> outside of the assembly, or to tell the compiler about
> the fact that there is an actual memory access if
> __put_user() was pointed at kernel memory.

Thank you so much, Arnd!

> 
> If you just copy from the arm64 version that uses an
> "r"(address) constraint instead of the "m"(*address)

"m" version is better than "r", usually can save one
instruction.
I will try to combine other constraints with "r" to
see whether we can still generate the sd with offset
instruction. If can't, seems sticking with "m" and keeping
output constraints is better

> version, it should be fine for any user space access.

You only mention "user space access", so just curious, does
arm64 version still correctly work with below __put_kernel_nofault()
example?

> 
> The output constraint is technically still be needed
> if we have code like this one where we actually write to
> something in kernel space:
> 
> int f(void)
> {
>      int a = 1;
>      int b = 2;
>      __put_kernel_nofault(&a, &b, int, error);
>      return a;
> error:
>      return -EFAULT;
> }
> 
> In this case, __put_kernel_nofault() writes the value
> of b into a, but the compiler can safely assume that
> a is not changed by the assembly because there is no
> memory output, and would likely just return a constant '1'. 
> 
> For put_user(), this cannot happen because the compiler
> doesn't know anything about the contents of the __user
> pointer. For __put_kernel_nofault(), we rely on the
> callers never using it on pointers they access, which
> is probably a reasonable assumption, but not entirely
> correct.
> 
>      Arnd

Well explained! Thanks a lot.

Re: [PATCH 1/4] riscv: implement user_access_begin and families

[Cyril Bur]

On 25/6/2024 2:04 pm, Jisheng Zhang wrote:
> Currently, when a function like strncpy_from_user() is called,
> the userspace access protection is disabled and enabled
> for every word read.
> 
> By implementing user_access_begin and families, the protection
> is disabled at the beginning of the copy and enabled at the end.
> 
> The __inttype macro is borrowed from x86 implementation.
> 

Beat me to it, I've written an almost identical patch. The performance 
improvement where the unsafe_ variants are used is very good even 
without the rest of the series.

Reviewed-by: Cyril Bur <cyrilbur@tenstorrent.com>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   arch/riscv/include/asm/uaccess.h | 63 ++++++++++++++++++++++++++++++++
>   1 file changed, 63 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
> index 72ec1d9bd3f3..09d4ca37522c 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -28,6 +28,19 @@
>   #define __disable_user_access()							\
>   	__asm__ __volatile__ ("csrc sstatus, %0" : : "r" (SR_SUM) : "memory")
>   
> +/*
> + * This is the smallest unsigned integer type that can fit a value
> + * (up to 'long long')
> + */
> +#define __inttype(x) __typeof__(		\
> +	__typefits(x,char,			\
> +	  __typefits(x,short,			\
> +	    __typefits(x,int,			\
> +	      __typefits(x,long,0ULL)))))
> +
> +#define __typefits(x,type,not) \
> +	__builtin_choose_expr(sizeof(x)<=sizeof(type),(unsigned type)0,not)
> +
>   /*
>    * The exception table consists of pairs of addresses: the first is the
>    * address of an instruction that is allowed to fault, and the second is
> @@ -335,6 +348,56 @@ do {									\
>   		goto err_label;						\
>   } while (0)
>   
> +static __must_check __always_inline bool user_access_begin(const void __user *ptr, size_t len)
> +{
> +	if (unlikely(!access_ok(ptr,len)))
> +		return 0;
> +	__enable_user_access();
> +	return 1;
> +}
> +#define user_access_begin(a,b)	user_access_begin(a,b)
> +#define user_access_end()	__disable_user_access();
> +
> +static inline unsigned long user_access_save(void) { return 0UL; }
> +static inline void user_access_restore(unsigned long enabled) { }
> +
> +#define unsafe_put_user(x, ptr, label)	do {				\
> +	long __kr_err = 0;						\
> +	__put_user_nocheck(x, (ptr), __kr_err);				\
> +	if (__kr_err) goto label;					\
> +} while (0)
> +
> +#define unsafe_get_user(x, ptr, label)	do {				\
> +	long __kr_err = 0;						\
> +	__inttype(*(ptr)) __gu_val;					\
> +	__get_user_nocheck(__gu_val, (ptr), __kr_err);			\
> +	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
> +	if (__kr_err) goto label;					\
> +} while (0)
> +
> +/*
> + * We want the unsafe accessors to always be inlined and use
> + * the error labels - thus the macro games.
> + */
> +#define unsafe_copy_loop(dst, src, len, type, label)				\
> +	while (len >= sizeof(type)) {						\
> +		unsafe_put_user(*(type *)(src),(type __user *)(dst),label);	\
> +		dst += sizeof(type);						\
> +		src += sizeof(type);						\
> +		len -= sizeof(type);						\
> +	}
> +
> +#define unsafe_copy_to_user(_dst,_src,_len,label)			\
> +do {									\
> +	char __user *__ucu_dst = (_dst);				\
> +	const char *__ucu_src = (_src);					\
> +	size_t __ucu_len = (_len);					\
> +	unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u64, label);	\
> +	unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u32, label);	\
> +	unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u16, label);	\
> +	unsafe_copy_loop(__ucu_dst, __ucu_src, __ucu_len, u8, label);	\
> +} while (0)
> +
>   #else /* CONFIG_MMU */
>   #include <asm-generic/uaccess.h>
>   #endif /* CONFIG_MMU */

Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[Arnd Bergmann]

On Wed, Jun 26, 2024, at 18:02, Jisheng Zhang wrote:
>
> "m" version is better than "r", usually can save one
> instruction.
> I will try to combine other constraints with "r" to
> see whether we can still generate the sd with offset
> instruction. If can't, seems sticking with "m" and keeping
> output constraints is better

Ah, I see.

> You only mention "user space access", so just curious, does
> arm64 version still correctly work with below __put_kernel_nofault()
> example?

No, I think the example I gave would break for both x86 and arm64
without adding an output constraint.

My main concern about using an input constraint was that it
doesn't match what the code does. Maybe there is a way to
make it use the correct "=m" output when CONFIG_CC_HAS_ASM_GOTO_OUTPUT
is set but use either "r" or "m" inputs on older gcc releases.

After gcc-11 becomes the minimum in a few years, the hack can
be removed.

     Arnd

RE: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user

[David Laight]

From: Arnd Bergmann
> Sent: 26 June 2024 15:25
...
> If you just copy from the arm64 version that uses an
> "r"(address) constraint instead of the "m"(*address)
> version, it should be fine for any user space access.

Arm certainly has 'reg+offset' addressing and I'd have thought
the RISC-V would have it as well.

I'd guess that the compiler also knows when the offset is too big.

Probably noticeable when code is accessing structures in user memory.

OTOH I can't remember if "m" implies a memory clobber?
For user copies the memory clobber isn't needed and not having it
may well allow better instruction scheduling.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Linus Torvalds]

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

On Tue, 25 Jun 2024 at 11:12, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But yes, it would be lovely if we did things as "implement the
> low-level accessor functions and we'll wrap them for the generic case"
> rather than have every architecture have to do the wrapping..

Btw, to do that _well_, we need to expand on the user access functions
a bit more.

In particular, we can already implement "get_user()" on top of
user_access_begin() and friends something like this:

  #define get_user(x,ptr) ({                                    \
        __label__ Efault_read;                                  \
        long __err;                                             \
        __typeof__(ptr) __ptr = (ptr);                          \
        if (likely(user_access_begin(__ptr, sizeof(x)))) {      \
                unsafe_get_user(x, __ptr, Efault_read);         \
                user_access_end();                              \
                __err = 0;                                      \
        } else {                                                \
                if (0) {                                        \
Efault_read:            user_access_end();                      \
                }                                               \
                x = (__typeof__(x))0;                           \
                __err = -EFAULT;                                \
        }                                                       \
        __err; })

and it doesn't generate _horrible_ code. It looks pretty bad, but all
the error handling goes out-of-line, so on x86-64 (without debug
options) it generates code something like this:

        test   %rdi,%rdi
        js     <cap_validate_magic+0x98>
        stac
        lfence
        mov    (%rdi),%ecx
        clac

which is certainly not horrid. But it's not great, because that lfence
ends up really screwing up the pipeline.

The manually coded out-of-line code generates this instead:

        mov    %rax,%rdx
        sar    $0x3f,%rdx
        or     %rdx,%rax
        stac
        movzbl (%rax),%edx
        xor    %eax,%eax
        clac
        ret

because it doesn't do a conditional branch (with the required spectre
thing), but instead does the address as a data dependency and knows
that "all bits set" if the address was negative will cause a page
fault.

But we *can* get the user accesses to do the same with a slight
expansion of user_access_begin():

        stac
        mov    %rdi,%rax
        sar    $0x3f,%rax
        or     %rdi,%rax
        mov    (%rax),%eax
        clac

by just introducing a notion of "masked_user_access". The get_user()
macro part would look like this:

        __typeof__(ptr) __ptr;                                  \
        __ptr = masked_user_access_begin(ptr);                  \
        if (1) {                                                \
                unsafe_get_user(x, __ptr, Efault_read);         \
                user_access_end();                              \

and the patch to implement this is attached. I've had it around for a
while, but I don't know how many architectures can do this.

Note this part of the commit message:

  This model only works for dense accesses that start at 'src' and on
  architectures that have a guard region that is guaranteed to fault in
  between the user space and the kernel space area.

which is true on x86-64, but that "guard region" thing might not be
true everywhere.

Will/Catalin - would that

        src = masked_user_access_begin(src);

work on arm64? The code does do something like that with
__uaccess_mask_ptr() already, but at least currently it doesn't do the
"avoid conditional entirely", the masking is just in _addition_ to the
access_ok().

                 Linus

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

From 6b2c9a69efc21b9e6e0497a5661273f6fbe204b2 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 8 Apr 2024 20:04:58 -0700
Subject: [PATCH] x86: support user address masking instead of non-speculative
 conditional

The Spectre-v1 mitigations made "access_ok()" much more expensive, since
it has to serialize execution with the test for a valid user address.

All the normal user copy routines avoid this by just masking the user
address with a data-dependent mask instead, but the fast
"unsafe_user_read()" kind of patterms that were supposed to be a fast
case got slowed down.

This introduces a notion of using

	src = masked_user_access_begin(src);

to do the user address sanity using a data-dependent mask instead of the
more traditional conditional

	if (user_read_access_begin(src, len)) {

model.

This model only works for dense accesses that start at 'src' and on
architectures that have a guard region that is guaranteed to fault in
between the user space and the kernel space area.

With this, the user access doesn't need to be manually checked, because
a bad address is guaranteed to fault (by some architecture masking
trick: on x86-64 this involves just turning an invalid user address into
all ones, since we don't map the top of address space).

This only converts a couple of examples for now.  Example x86-64 code
generation for loading two words from user space:

        stac
        mov    %rax,%rcx
        sar    $0x3f,%rcx
        or     %rax,%rcx
        mov    (%rcx),%r13
        mov    0x8(%rcx),%r14
        clac

where all the error handling and -EFAULT is now purely handled out of
line by the exception path.

Of course, if the micro-architecture does badly at 'clac' and 'stac',
the above is still pitifully slow.  But at least we did as well as we
could.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 arch/x86/include/asm/uaccess_64.h | 8 ++++++++
 fs/select.c                       | 4 +++-
 include/linux/uaccess.h           | 7 +++++++
 lib/strncpy_from_user.c           | 9 +++++++++
 lib/strnlen_user.c                | 9 +++++++++
 5 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index 04789f45ab2b..a10149a96d9e 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -53,6 +53,14 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm,
  */
 #define valid_user_address(x) ((__force long)(x) >= 0)
 
+/*
+ * Masking the user address is an alternative to a conditional
+ * user_access_begin that can avoid the fencing. This only works
+ * for dense accesses starting at the address.
+ */
+#define mask_user_address(x) ((typeof(x))((long)(x)|((long)(x)>>63)))
+#define masked_user_access_begin(x) ({ __uaccess_begin(); mask_user_address(x); })
+
 /*
  * User pointers can have tag bits on x86-64.  This scheme tolerates
  * arbitrary values in those bits rather then masking them off.
diff --git a/fs/select.c b/fs/select.c
index 9515c3fa1a03..bc185d111436 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -780,7 +780,9 @@ static inline int get_sigset_argpack(struct sigset_argpack *to,
 {
 	// the path is hot enough for overhead of copy_from_user() to matter
 	if (from) {
-		if (!user_read_access_begin(from, sizeof(*from)))
+		if (can_do_masked_user_access())
+			from = masked_user_access_begin(from);
+		else if (!user_read_access_begin(from, sizeof(*from)))
 			return -EFAULT;
 		unsafe_get_user(to->p, &from->p, Efault);
 		unsafe_get_user(to->size, &from->size, Efault);
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 3064314f4832..f18371f6cf36 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -32,6 +32,13 @@
 })
 #endif
 
+#ifdef masked_user_access_begin
+ #define can_do_masked_user_access() 1
+#else
+ #define can_do_masked_user_access() 0
+ #define masked_user_access_begin(src) NULL
+#endif
+
 /*
  * Architectures should provide two primitives (raw_copy_{to,from}_user())
  * and get rid of their private instances of copy_{to,from}_user() and
diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index 6432b8c3e431..989a12a67872 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -120,6 +120,15 @@ long strncpy_from_user(char *dst, const char __user *src, long count)
 	if (unlikely(count <= 0))
 		return 0;
 
+	if (can_do_masked_user_access()) {
+		long retval;
+
+		src = masked_user_access_begin(src);
+		retval = do_strncpy_from_user(dst, src, count, count);
+		user_read_access_end();
+		return retval;
+	}
+
 	max_addr = TASK_SIZE_MAX;
 	src_addr = (unsigned long)untagged_addr(src);
 	if (likely(src_addr < max_addr)) {
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index feeb935a2299..6e489f9e90f1 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -96,6 +96,15 @@ long strnlen_user(const char __user *str, long count)
 	if (unlikely(count <= 0))
 		return 0;
 
+	if (can_do_masked_user_access()) {
+		long retval;
+
+		str = masked_user_access_begin(str);
+		retval = do_strnlen_user(str, count, count);
+		user_read_access_end();
+		return retval;
+	}
+
 	max_addr = TASK_SIZE_MAX;
 	src_addr = (unsigned long)untagged_addr(str);
 	if (likely(src_addr < max_addr)) {

Re: [PATCH 3/4] riscv: uaccess: use 'asm goto' for put_user()

[kernel test robot]

Hi Jisheng,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.10-rc6 next-20240703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jisheng-Zhang/riscv-implement-user_access_begin-and-families/20240626-005352
base:   linus/master
patch link:    https://lore.kernel.org/r/20240625040500.1788-4-jszhang%40kernel.org
patch subject: [PATCH 3/4] riscv: uaccess: use 'asm goto' for put_user()
config: riscv-randconfig-r121-20240705 (https://download.01.org/0day-ci/archive/20240705/202407051058.kE7ADWxJ-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240705/202407051058.kE7ADWxJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407051058.kE7ADWxJ-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:13,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/linux/compat.h:17,
                    from arch/riscv/include/asm/elf.h:12,
                    from include/linux/elf.h:6,
                    from include/linux/module.h:19,
                    from include/linux/device/driver.h:21,
                    from include/linux/device.h:32,
                    from include/linux/node.h:18,
                    from include/linux/cpu.h:17,
                    from arch/riscv/kernel/process.c:10:
   arch/riscv/kernel/process.c: In function 'get_unalign_ctl':
>> arch/riscv/include/asm/uaccess.h:211:46: error: expected ')' before ':' token
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:301:17: note: in expansion of macro '__put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^~~~~~~~~~
   arch/riscv/kernel/process.c:57:16: note: in expansion of macro 'put_user'
      57 |         return put_user(tsk->thread.align_ctl, (unsigned long __user *)adr);
         |                ^~~~~~~~
   arch/riscv/include/asm/uaccess.h:202:30: note: to match this '('
     202 |         __asm__ __volatile__ (                                  \
         |                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:301:17: note: in expansion of macro '__put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^~~~~~~~~~
   arch/riscv/kernel/process.c:57:16: note: in expansion of macro 'put_user'
      57 |         return put_user(tsk->thread.align_ctl, (unsigned long __user *)adr);
         |                ^~~~~~~~
--
   In file included from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:13,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/linux/compat.h:17,
                    from arch/riscv/kernel/signal.c:9:
   arch/riscv/kernel/signal.c: In function 'setup_sigcontext':
>> arch/riscv/include/asm/uaccess.h:211:46: error: expected ')' before ':' token
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/kernel/signal.c:290:16: note: in expansion of macro '__put_user'
     290 |         err |= __put_user(0, &sc->sc_extdesc.reserved);
         |                ^~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:202:30: note: to match this '('
     202 |         __asm__ __volatile__ (                                  \
         |                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/kernel/signal.c:290:16: note: in expansion of macro '__put_user'
     290 |         err |= __put_user(0, &sc->sc_extdesc.reserved);
         |                ^~~~~~~~~~
>> arch/riscv/include/asm/uaccess.h:211:46: error: expected ')' before ':' token
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/kernel/signal.c:292:16: note: in expansion of macro '__put_user'
     292 |         err |= __put_user(END_MAGIC, &sc_ext_ptr->magic);
         |                ^~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:202:30: note: to match this '('
     202 |         __asm__ __volatile__ (                                  \
         |                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/kernel/signal.c:292:16: note: in expansion of macro '__put_user'
     292 |         err |= __put_user(END_MAGIC, &sc_ext_ptr->magic);
         |                ^~~~~~~~~~
>> arch/riscv/include/asm/uaccess.h:211:46: error: expected ')' before ':' token
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/kernel/signal.c:293:16: note: in expansion of macro '__put_user'
     293 |         err |= __put_user(END_HDR_SIZE, &sc_ext_ptr->size);
         |                ^~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:202:30: note: to match this '('
     202 |         __asm__ __volatile__ (                                  \
         |                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/kernel/signal.c:293:16: note: in expansion of macro '__put_user'
     293 |         err |= __put_user(END_HDR_SIZE, &sc_ext_ptr->size);
         |                ^~~~~~~~~~
   arch/riscv/kernel/signal.c: In function 'setup_rt_frame':
>> arch/riscv/include/asm/uaccess.h:211:46: error: expected ')' before ':' token
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/kernel/signal.c:336:16: note: in expansion of macro '__put_user'
     336 |         err |= __put_user(0, &frame->uc.uc_flags);
         |                ^~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:202:30: note: to match this '('
     202 |         __asm__ __volatile__ (                                  \
         |                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/kernel/signal.c:336:16: note: in expansion of macro '__put_user'
     336 |         err |= __put_user(0, &frame->uc.uc_flags);
         |                ^~~~~~~~~~
>> arch/riscv/include/asm/uaccess.h:211:46: error: expected ')' before ':' token
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/kernel/signal.c:337:16: note: in expansion of macro '__put_user'
     337 |         err |= __put_user(NULL, &frame->uc.uc_link);
         |                ^~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:202:30: note: to match this '('
     202 |         __asm__ __volatile__ (                                  \
         |                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/kernel/signal.c:337:16: note: in expansion of macro '__put_user'
     337 |         err |= __put_user(NULL, &frame->uc.uc_link);
         |                ^~~~~~~~~~
--
   In file included from include/linux/uaccess.h:11,
                    from include/linux/sched/task.h:13,
                    from include/linux/sched/signal.h:9,
                    from include/linux/rcuwait.h:6,
                    from include/linux/percpu-rwsem.h:7,
                    from include/linux/fs.h:33,
                    from include/uapi/linux/aio_abi.h:31,
                    from include/linux/syscalls.h:82,
                    from arch/riscv/kernel/sys_hwprobe.c:7:
   arch/riscv/kernel/sys_hwprobe.c: In function 'hwprobe_get_values':
>> arch/riscv/include/asm/uaccess.h:211:46: error: expected ')' before ':' token
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:301:17: note: in expansion of macro '__put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^~~~~~~~~~
   arch/riscv/kernel/sys_hwprobe.c:278:23: note: in expansion of macro 'put_user'
     278 |                 ret = put_user(pair.key, &pairs->key);
         |                       ^~~~~~~~
   arch/riscv/include/asm/uaccess.h:202:30: note: to match this '('
     202 |         __asm__ __volatile__ (                                  \
         |                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:301:17: note: in expansion of macro '__put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^~~~~~~~~~
   arch/riscv/kernel/sys_hwprobe.c:278:23: note: in expansion of macro 'put_user'
     278 |                 ret = put_user(pair.key, &pairs->key);
         |                       ^~~~~~~~
>> arch/riscv/include/asm/uaccess.h:211:46: error: expected ')' before ':' token
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:301:17: note: in expansion of macro '__put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^~~~~~~~~~
   arch/riscv/kernel/sys_hwprobe.c:280:31: note: in expansion of macro 'put_user'
     280 |                         ret = put_user(pair.value, &pairs->value);
         |                               ^~~~~~~~
   arch/riscv/include/asm/uaccess.h:202:30: note: to match this '('
     202 |         __asm__ __volatile__ (                                  \
         |                              ^
   arch/riscv/include/asm/uaccess.h:228:17: note: in expansion of macro '__put_user_8'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:238:9: note: in expansion of macro '__put_user_nocheck'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:274:9: note: in expansion of macro '__put_user_error'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:301:17: note: in expansion of macro '__put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^~~~~~~~~~
   arch/riscv/kernel/sys_hwprobe.c:280:31: note: in expansion of macro 'put_user'
     280 |                         ret = put_user(pair.value, &pairs->value);
         |                               ^~~~~~~~


vim +211 arch/riscv/include/asm/uaccess.h

   193	
   194	#ifdef CONFIG_64BIT
   195	#define __put_user_8(x, ptr, label) \
   196		__put_user_asm("sd", x, ptr, label)
   197	#else /* !CONFIG_64BIT */
   198	#define __put_user_8(x, ptr, label)				\
   199	do {								\
   200		u32 __user *__ptr = (u32 __user *)(ptr);		\
   201		u64 __x = (__typeof__((x)-(x)))(x);			\
   202		__asm__ __volatile__ (					\
   203			"1:\n"						\
   204			"	sw %z0, %2\n"				\
   205			"2:\n"						\
   206			"	sw %z1, %3\n"				\
   207			_ASM_EXTABLE(1b, %l4)				\
   208			_ASM_EXTABLE(2b, %l4)				\
   209			: : "rJ" (__x), "rJ" (__x >> 32),		\
   210				"m" (__ptr[__LSW]),			\
 > 211				"m" (__ptr[__MSW]) : : label);		\
   212	} while (0)
   213	#endif /* CONFIG_64BIT */
   214	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/4] riscv: uaccess: use 'asm goto output' for get_user

[kernel test robot]

Hi Jisheng,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.10-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jisheng-Zhang/riscv-implement-user_access_begin-and-families/20240626-005352
base:   linus/master
patch link:    https://lore.kernel.org/r/20240625040500.1788-5-jszhang%40kernel.org
patch subject: [PATCH 4/4] riscv: uaccess: use 'asm goto output' for get_user
config: riscv-randconfig-r121-20240705 (https://download.01.org/0day-ci/archive/20240705/202407051159.ArkAPA6L-lkp@intel.com/config)
compiler: riscv32-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240705/202407051159.ArkAPA6L-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407051159.ArkAPA6L-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/compiler_types.h:174,
                    from <command-line>:
   net/rfkill/core.c: In function 'rfkill_fop_ioctl':
>> arch/riscv/include/asm/uaccess.h:104:26: error: '__lo' undeclared (first use in this function)
     104 |                 : "=&r" (__lo), "=r" (__hi)                     \
         |                          ^~~~
   include/linux/compiler-gcc.h:84:32: note: in definition of macro 'asm_goto_output'
      84 |         do { asm volatile goto(x); asm (""); } while (0)
         |                                ^
   arch/riscv/include/asm/uaccess.h:148:17: note: in expansion of macro '__get_user_8'
     148 |                 __get_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:159:9: note: in expansion of macro '__get_user_nocheck'
     159 |         __get_user_nocheck(x, ptr, __gu_failed);                        \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:196:9: note: in expansion of macro '__get_user_error'
     196 |         __get_user_error(__gu_val, __gu_ptr, __gu_err);         \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:226:17: note: in expansion of macro '__get_user'
     226 |                 __get_user((x), __p) :                          \
         |                 ^~~~~~~~~~
   net/rfkill/core.c:1373:21: note: in expansion of macro 'get_user'
    1373 |                 if (get_user(size, (__u32 __user *)arg)) {
         |                     ^~~~~~~~
   arch/riscv/include/asm/uaccess.h:104:26: note: each undeclared identifier is reported only once for each function it appears in
     104 |                 : "=&r" (__lo), "=r" (__hi)                     \
         |                          ^~~~
   include/linux/compiler-gcc.h:84:32: note: in definition of macro 'asm_goto_output'
      84 |         do { asm volatile goto(x); asm (""); } while (0)
         |                                ^
   arch/riscv/include/asm/uaccess.h:148:17: note: in expansion of macro '__get_user_8'
     148 |                 __get_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:159:9: note: in expansion of macro '__get_user_nocheck'
     159 |         __get_user_nocheck(x, ptr, __gu_failed);                        \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:196:9: note: in expansion of macro '__get_user_error'
     196 |         __get_user_error(__gu_val, __gu_ptr, __gu_err);         \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:226:17: note: in expansion of macro '__get_user'
     226 |                 __get_user((x), __p) :                          \
         |                 ^~~~~~~~~~
   net/rfkill/core.c:1373:21: note: in expansion of macro 'get_user'
    1373 |                 if (get_user(size, (__u32 __user *)arg)) {
         |                     ^~~~~~~~
>> arch/riscv/include/asm/uaccess.h:104:39: error: '__hi' undeclared (first use in this function)
     104 |                 : "=&r" (__lo), "=r" (__hi)                     \
         |                                       ^~~~
   include/linux/compiler-gcc.h:84:32: note: in definition of macro 'asm_goto_output'
      84 |         do { asm volatile goto(x); asm (""); } while (0)
         |                                ^
   arch/riscv/include/asm/uaccess.h:148:17: note: in expansion of macro '__get_user_8'
     148 |                 __get_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:159:9: note: in expansion of macro '__get_user_nocheck'
     159 |         __get_user_nocheck(x, ptr, __gu_failed);                        \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:196:9: note: in expansion of macro '__get_user_error'
     196 |         __get_user_error(__gu_val, __gu_ptr, __gu_err);         \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:226:17: note: in expansion of macro '__get_user'
     226 |                 __get_user((x), __p) :                          \
         |                 ^~~~~~~~~~
   net/rfkill/core.c:1373:21: note: in expansion of macro 'get_user'
    1373 |                 if (get_user(size, (__u32 __user *)arg)) {
         |                     ^~~~~~~~
>> arch/riscv/include/asm/uaccess.h:105:24: error: '__ptr' undeclared (first use in this function); did you mean '__p'?
     105 |                 : "m" (__ptr[__LSW]), "m" (__ptr[__MSW])        \
         |                        ^~~~~
   include/linux/compiler-gcc.h:84:32: note: in definition of macro 'asm_goto_output'
      84 |         do { asm volatile goto(x); asm (""); } while (0)
         |                                ^
   arch/riscv/include/asm/uaccess.h:148:17: note: in expansion of macro '__get_user_8'
     148 |                 __get_user_8((x), __gu_ptr, label);             \
         |                 ^~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:159:9: note: in expansion of macro '__get_user_nocheck'
     159 |         __get_user_nocheck(x, ptr, __gu_failed);                        \
         |         ^~~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:196:9: note: in expansion of macro '__get_user_error'
     196 |         __get_user_error(__gu_val, __gu_ptr, __gu_err);         \
         |         ^~~~~~~~~~~~~~~~
   arch/riscv/include/asm/uaccess.h:226:17: note: in expansion of macro '__get_user'
     226 |                 __get_user((x), __p) :                          \
         |                 ^~~~~~~~~~
   net/rfkill/core.c:1373:21: note: in expansion of macro 'get_user'
    1373 |                 if (get_user(size, (__u32 __user *)arg)) {
         |                     ^~~~~~~~


vim +/__lo +104 arch/riscv/include/asm/uaccess.h

    94	
    95	#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
    96	#define __get_user_8(x, ptr, label)				\
    97		asm_goto_output(					\
    98			"1:\n"						\
    99			"	lw %0, %2\n"				\
   100			"2:\n"						\
   101			"	lw %1, %3\n"				\
   102			_ASM_EXTABLE_UACCESS_ERR(1b, %l4, %0)		\
   103			_ASM_EXTABLE_UACCESS_ERR(2b, %l4, %0)		\
 > 104			: "=&r" (__lo), "=r" (__hi)			\
 > 105			: "m" (__ptr[__LSW]), "m" (__ptr[__MSW])	\
   106			: : label)
   107	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Will Deacon]

On Sun, Jun 30, 2024 at 09:59:36AM -0700, Linus Torvalds wrote:
> On Tue, 25 Jun 2024 at 11:12, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > But yes, it would be lovely if we did things as "implement the
> > low-level accessor functions and we'll wrap them for the generic case"
> > rather than have every architecture have to do the wrapping..
> 
> Btw, to do that _well_, we need to expand on the user access functions
> a bit more.

[...]

> Will/Catalin - would that
> 
>         src = masked_user_access_begin(src);
> 
> work on arm64? The code does do something like that with
> __uaccess_mask_ptr() already, but at least currently it doesn't do the
> "avoid conditional entirely", the masking is just in _addition_ to the
> access_ok().

I think we'd need to go back to our old __uaccess_mask_ptr()
implementation, where kernel addresses end up being forced to NULL. In
other words, revert 2305b809be93 ("arm64: uaccess: simplify
uaccess_mask_ptr()"). If we then want to drop the access_ok() entirely,
we'd probably want to use an address that lives between the two TTBRs
(i.e. in the "guard region" you mentioned above), just in case somebody
has fscked around with /proc/sys/vm/mmap_min_addr.

Will

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Linus Torvalds]

On Fri, 5 Jul 2024 at 04:25, Will Deacon <will@kernel.org> wrote:
>
> we'd probably want to use an address that lives between the two TTBRs
> (i.e. in the "guard region" you mentioned above), just in case somebody
> has fscked around with /proc/sys/vm/mmap_min_addr.

Yes, I don't want to use a NULL pointer and rely on mmap_min_addr.

For x86-64, we have two "guard regions" that can be used to generate
an address that is guaranteed to fault:

 - the kernel always lives in the "top bit set" part of the address
space (and any address tagging bits don't touch that part), and does
not map the highest virtual address because that's used for error
pointers, so the "all bits set" address always faults

 - the region between valid user addresses and kernel addresses is
also always going to fault, and we don't have them adjacent to each
other (unlike, for example, 32-bit i386, where the kernel address
space is directly adjacent to the top of user addresses)

So on x86-64, the simple solution is to just say "we know if the top
bit is clear, it cannot ever touch kernel code, and if the top bit is
set we have to make the address fault". So just duplicating the top
bit (with an arithmetic shift) and or'ing it with the low bits, we get
exactly what we want.

But my knowledge of arm64 is weak enough that while I am reading
assembly language and I know that instead of the top bit, it's bit55,
I don't know what the actual rules for the translation table registers
are.

If the all-bits-set address is guaranteed to always trap, then arm64
could just use the same thing x86 does (just duplicating bit 55
instead of the sign bit)?

               Linus

Re: [PATCH 3/4] riscv: uaccess: use 'asm goto' for put_user()

[kernel test robot]

Hi Jisheng,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.10-rc6 next-20240703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jisheng-Zhang/riscv-implement-user_access_begin-and-families/20240626-005352
base:   linus/master
patch link:    https://lore.kernel.org/r/20240625040500.1788-4-jszhang%40kernel.org
patch subject: [PATCH 3/4] riscv: uaccess: use 'asm goto' for put_user()
config: riscv-randconfig-r071-20240705 (https://download.01.org/0day-ci/archive/20240706/202407060732.wjARQ4O7-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project a0c6b8aef853eedaa0980f07c0a502a5a8a9740e)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240706/202407060732.wjARQ4O7-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407060732.wjARQ4O7-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from block/ioctl.c:4:
   In file included from include/linux/blkdev.h:9:
   In file included from include/linux/blk_types.h:10:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/riscv/include/asm/cacheflush.h:9:
   In file included from include/linux/mm.h:2253:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> block/ioctl.c:236:9: error: expected ')'
     236 |         return put_user(val, argp);
         |                ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   block/ioctl.c:241:9: error: expected ')'
     241 |         return put_user(val, argp);
         |                ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   block/ioctl.c:246:9: error: expected ')'
     246 |         return put_user(val, argp);
         |                ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   block/ioctl.c:251:9: error: expected ')'
     251 |         return put_user(val, argp);
         |                ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   block/ioctl.c:256:9: error: expected ')'
     256 |         return put_user(val, argp);
         |                ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   block/ioctl.c:261:9: error: expected ')'
     261 |         return put_user(val, argp);
         |                ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
--
   In file included from block/bsg.c:8:
   In file included from include/linux/blkdev.h:9:
   In file included from include/linux/blk_types.h:10:
   In file included from include/linux/bvec.h:10:
   In file included from include/linux/highmem.h:8:
   In file included from include/linux/cacheflush.h:5:
   In file included from arch/riscv/include/asm/cacheflush.h:9:
   In file included from include/linux/mm.h:2253:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> block/bsg.c:89:9: error: expected ')'
      89 |         return put_user(READ_ONCE(bd->max_queue), uarg);
         |                ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   block/bsg.c:125:10: error: expected ')'
     125 |                 return put_user(30527, intp);
         |                        ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   block/bsg.c:127:10: error: expected ')'
     127 |                 return put_user(0, intp);
         |                        ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   block/bsg.c:129:10: error: expected ')'
     129 |                 return put_user(0, intp);
         |                        ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   block/bsg.c:138:10: error: expected ')'
     138 |                 return put_user(min(bd->reserved_size, queue_max_bytes(q)),
         |                        ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   block/bsg.c:149:10: error: expected ')'
     149 |                 return put_user(1, intp);
         |                        ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
--
   In file included from fs/read_write.c:14:
   In file included from include/linux/fsnotify.h:16:
   In file included from include/linux/audit.h:13:
   In file included from include/linux/ptrace.h:10:
   In file included from include/linux/pid_namespace.h:7:
   In file included from include/linux/mm.h:2253:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> fs/read_write.c:1340:16: error: expected ')'
    1340 |                 if (unlikely(put_user(pos, offset)))
         |                              ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   fs/read_write.c:1357:16: error: expected ')'
    1357 |                 if (unlikely(put_user(pos, offset)))
         |                              ^
   arch/riscv/include/asm/uaccess.h:301:3: note: expanded from macro 'put_user'
     301 |                 __put_user((x), __p) :                          \
         |                 ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   1 warning and 2 errors generated.
--
   In file included from fs/file_table.c:17:
   In file included from include/linux/security.h:33:
   In file included from include/linux/mm.h:2253:
   include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     514 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   In file included from fs/file_table.c:19:
>> include/linux/eventpoll.h:81:6: error: expected ')'
      81 |         if (__put_user(revents, &uevent->events) ||
         |             ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   In file included from fs/file_table.c:19:
   include/linux/eventpoll.h:82:6: error: expected ')'
      82 |             __put_user(data, &uevent->data))
         |             ^
   arch/riscv/include/asm/uaccess.h:274:2: note: expanded from macro '__put_user'
     274 |         __put_user_error(__val, __gu_ptr, __pu_err);            \
         |         ^
   arch/riscv/include/asm/uaccess.h:238:2: note: expanded from macro '__put_user_error'
     238 |         __put_user_nocheck(x, ptr, err_label);                  \
         |         ^
   arch/riscv/include/asm/uaccess.h:228:3: note: expanded from macro '__put_user_nocheck'
     228 |                 __put_user_8((x), __gu_ptr, label);             \
         |                 ^
   arch/riscv/include/asm/uaccess.h:211:25: note: expanded from macro '__put_user_8'
     211 |                         "m" (__ptr[__MSW]) : : label);          \
         |                                              ^
   1 warning and 2 errors generated.
..


vim +236 block/ioctl.c

66ba32dc167202c block/ioctl.c         Martin K. Petersen 2012-09-18  233  
9b81648cb5e3ae7 block/ioctl.c         Arnd Bergmann      2019-11-29  234  static int put_ushort(unsigned short __user *argp, unsigned short val)
^1da177e4c3f415 drivers/block/ioctl.c Linus Torvalds     2005-04-16  235  {
9b81648cb5e3ae7 block/ioctl.c         Arnd Bergmann      2019-11-29 @236  	return put_user(val, argp);
^1da177e4c3f415 drivers/block/ioctl.c Linus Torvalds     2005-04-16  237  }
^1da177e4c3f415 drivers/block/ioctl.c Linus Torvalds     2005-04-16  238  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Will Deacon]

On Fri, Jul 05, 2024 at 10:58:29AM -0700, Linus Torvalds wrote:
> On Fri, 5 Jul 2024 at 04:25, Will Deacon <will@kernel.org> wrote:
> >
> > we'd probably want to use an address that lives between the two TTBRs
> > (i.e. in the "guard region" you mentioned above), just in case somebody
> > has fscked around with /proc/sys/vm/mmap_min_addr.
> 
> Yes, I don't want to use a NULL pointer and rely on mmap_min_addr.
> 
> For x86-64, we have two "guard regions" that can be used to generate
> an address that is guaranteed to fault:
> 
>  - the kernel always lives in the "top bit set" part of the address
> space (and any address tagging bits don't touch that part), and does
> not map the highest virtual address because that's used for error
> pointers, so the "all bits set" address always faults
> 
>  - the region between valid user addresses and kernel addresses is
> also always going to fault, and we don't have them adjacent to each
> other (unlike, for example, 32-bit i386, where the kernel address
> space is directly adjacent to the top of user addresses)

I think we're very similar on arm64. The kernel lives at the top (i.e.
virtual address space descends below 0) and is mapped by TTBR1.
Userspace lives at the bottom (i.e. virtual address space ascends from
0) and is mapped by TTBR0. There's an unaddressable gap in the middle
and it's bit 55 of the address which determines user vs kernel (well, it
selects the TTBR to be precise).

The kernel doesn't map the top 8MiB of its VA space, although talking to
Mark, it sounds like we might not be as robust as x86 if there's a stray
dereference of an unaligned error pointer that straddles 0. He can
elaborate, but we probably can't rely on a pointer of all-ones faulting
safely for the same reason.

> So on x86-64, the simple solution is to just say "we know if the top
> bit is clear, it cannot ever touch kernel code, and if the top bit is
> set we have to make the address fault". So just duplicating the top
> bit (with an arithmetic shift) and or'ing it with the low bits, we get
> exactly what we want.
> 
> But my knowledge of arm64 is weak enough that while I am reading
> assembly language and I know that instead of the top bit, it's bit55,
> I don't know what the actual rules for the translation table registers
> are.
> 
> If the all-bits-set address is guaranteed to always trap, then arm64
> could just use the same thing x86 does (just duplicating bit 55
> instead of the sign bit)?

Perhaps we could just force accesses with bit 55 set to the address
'1UL << 55'? That should sit slap bang in the middle of the guard
region between the TTBRs and I think it would resolve any issues we may
have with wrapping. It still means effectively reverting 2305b809be93
("arm64: uaccess: simplify uaccess_mask_ptr()"), though.

Dunno. Mark, Catalin, what do you guys reckon?

Will

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Mark Rutland]

On Fri, Jul 05, 2024 at 10:58:29AM -0700, Linus Torvalds wrote:
> On Fri, 5 Jul 2024 at 04:25, Will Deacon <will@kernel.org> wrote:
> >
> > we'd probably want to use an address that lives between the two TTBRs
> > (i.e. in the "guard region" you mentioned above), just in case somebody
> > has fscked around with /proc/sys/vm/mmap_min_addr.
> 
> Yes, I don't want to use a NULL pointer and rely on mmap_min_addr.
> 
> For x86-64, we have two "guard regions" that can be used to generate
> an address that is guaranteed to fault:
> 
>  - the kernel always lives in the "top bit set" part of the address
> space (and any address tagging bits don't touch that part), and does
> not map the highest virtual address because that's used for error
> pointers, so the "all bits set" address always faults

The same should be true on arm64, though I'm not immediately sure if we
explicitly reserve that VA region -- if we don't, then we should.

>  - the region between valid user addresses and kernel addresses is
> also always going to fault, and we don't have them adjacent to each
> other (unlike, for example, 32-bit i386, where the kernel address
> space is directly adjacent to the top of user addresses)

Today we have a gap between the TTBR0 and TTBR1 VA ranges in all
configurations, but in future (with the new FEAT_D128 page table format)
we will have configurations where there's no gap between the two ranges.

> So on x86-64, the simple solution is to just say "we know if the top
> bit is clear, it cannot ever touch kernel code, and if the top bit is
> set we have to make the address fault". So just duplicating the top
> bit (with an arithmetic shift) and or'ing it with the low bits, we get
> exactly what we want.
> 
> But my knowledge of arm64 is weak enough that while I am reading
> assembly language and I know that instead of the top bit, it's bit55,
> I don't know what the actual rules for the translation table registers
> are.
> 
> If the all-bits-set address is guaranteed to always trap, then arm64
> could just use the same thing x86 does (just duplicating bit 55
> instead of the sign bit)?

I think something of that shape can work (see below). There are a couple
of things that make using all-ones unsafe:

1) Non-faulting parts of a misaligned load/store can occur *before* the
   fault is raised. If you have two pages where one of which is writable
   and the other of which is not writeable (in either order), a store
   which straddles those pages can write to the writeable page before
   raising a fault on the non-writeable page.

   I've seen this behaviour on real HW, and IIUC this is fairly common.

2) Loads/stores which wrap past 0xFFFF_FFFF_FFFF_FFFF access bytes at
   UNKNOWN addresses. An N-byte store at 0xFFFF_FFFF_FFFF_FFFF may write
   to N-1 bytes at an arbitrary address which is not
   0x0000_0000_0000_0000.

   In the latest ARM ARM (K.a), this is described tersely in section
   K1.2.9 "Out of range virtual address".

   That can be found at:

   https://developer.arm.com/documentation/ddi0487/ka/?lang=en

   I'm aware of implementation styles where that address is not zero and
   can be a TTBR1 (kernel) address.

Given that, we'd need to avoid all-ones, but provided we know that the
first access using the pointer will be limited to PAGE_SIZE bytes past
the pointer, we could round down the bad pointer to be somewhere within
the error pointer page, e.g.

	SBFX <mask>, <ptr>, #55, #1
	ORR  <ptr>, <ptr>, <mask>
	BIC <ptr>, <ptr>, <mask>, lsr #(64 - PAGE_SHIFT)

That last `BIC` instructions is "BIt Clear" AKA "AND NOT". When bit 55
is one that will clear the lower bits to round down to a page boundary,
and when bit 55 is zero it will have no effect (as it'll be an AND with
all-ones).

Thanks,
Mark.

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Mark Rutland]

On Mon, Jul 08, 2024 at 02:52:43PM +0100, Will Deacon wrote:
> On Fri, Jul 05, 2024 at 10:58:29AM -0700, Linus Torvalds wrote:
> > On Fri, 5 Jul 2024 at 04:25, Will Deacon <will@kernel.org> wrote:
> > So on x86-64, the simple solution is to just say "we know if the top
> > bit is clear, it cannot ever touch kernel code, and if the top bit is
> > set we have to make the address fault". So just duplicating the top
> > bit (with an arithmetic shift) and or'ing it with the low bits, we get
> > exactly what we want.
> > 
> > But my knowledge of arm64 is weak enough that while I am reading
> > assembly language and I know that instead of the top bit, it's bit55,
> > I don't know what the actual rules for the translation table registers
> > are.
> > 
> > If the all-bits-set address is guaranteed to always trap, then arm64
> > could just use the same thing x86 does (just duplicating bit 55
> > instead of the sign bit)?
> 
> Perhaps we could just force accesses with bit 55 set to the address
> '1UL << 55'? That should sit slap bang in the middle of the guard
> region between the TTBRs

Yep, that'll work until we handle FEAT_D128 where (1UL << 55) will be
the start of the TTBR1 range in some configurations.

> and I think it would resolve any issues we may have with wrapping. It
> still means effectively reverting 2305b809be93 ("arm64: uaccess:
> simplify uaccess_mask_ptr()"), though.

If we do bring that back, it'd be nice if we could do that without the
CSEL+CSDB, as the CSDB is liable to be expensive on some parts (e.g.
it's an alias of DSB on older designs).

> Dunno. Mark, Catalin, what do you guys reckon?

I think there's a slight variant of the x86 approach that might work,
I've posted in my reply at

  https://lore.kernel.org/lkml/ZowD3LQT_KTz2g4X@J2N7QTR9R3/

Mark.

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Will Deacon]

On Mon, Jul 08, 2024 at 04:30:52PM +0100, Mark Rutland wrote:
> On Mon, Jul 08, 2024 at 02:52:43PM +0100, Will Deacon wrote:
> > On Fri, Jul 05, 2024 at 10:58:29AM -0700, Linus Torvalds wrote:
> > > On Fri, 5 Jul 2024 at 04:25, Will Deacon <will@kernel.org> wrote:
> > > So on x86-64, the simple solution is to just say "we know if the top
> > > bit is clear, it cannot ever touch kernel code, and if the top bit is
> > > set we have to make the address fault". So just duplicating the top
> > > bit (with an arithmetic shift) and or'ing it with the low bits, we get
> > > exactly what we want.
> > > 
> > > But my knowledge of arm64 is weak enough that while I am reading
> > > assembly language and I know that instead of the top bit, it's bit55,
> > > I don't know what the actual rules for the translation table registers
> > > are.
> > > 
> > > If the all-bits-set address is guaranteed to always trap, then arm64
> > > could just use the same thing x86 does (just duplicating bit 55
> > > instead of the sign bit)?
> > 
> > Perhaps we could just force accesses with bit 55 set to the address
> > '1UL << 55'? That should sit slap bang in the middle of the guard
> > region between the TTBRs
> 
> Yep, that'll work until we handle FEAT_D128 where (1UL << 55) will be
> the start of the TTBR1 range in some configurations.
> 
> > and I think it would resolve any issues we may have with wrapping. It
> > still means effectively reverting 2305b809be93 ("arm64: uaccess:
> > simplify uaccess_mask_ptr()"), though.
> 
> If we do bring that back, it'd be nice if we could do that without the
> CSEL+CSDB, as the CSDB is liable to be expensive on some parts (e.g.
> it's an alias of DSB on older designs).

DSB?! Are you sure? I thought it was basically a NOP for everybody apart
from a small subset of implementations.

Will

Re: [PATCH 0/4] riscv: uaccess: optimizations

[Palmer Dabbelt]

On Mon, 24 Jun 2024 21:04:56 PDT (-0700), jszhang@kernel.org wrote:
> This series tries to optimize riscv uaccess in the following way:
>
> patch1 implement the user_access_begin and families, I.E the unsafe
> accessors. when a function like strncpy_from_user() is called,
> the userspace access protection is disabled and enabled for every
> word read. After patch1, the protection is disabled at the beginning
> of the copy and enabled at the end.
>
> patch2 is a simple clean up
>
> patch3 uses 'asm goto' for put_user()
> patch4 uses 'asm goto output' for get_user()
>
> Both patch3 and patch4 are based on the fact -- 'asm goto' and
> 'asm goto output'generates noticeably better code since we don't need
> to test the error etc, the exception just jumps to the error handling
> directly.
>
>
> Jisheng Zhang (4):
>   riscv: implement user_access_begin and families
>   riscv: uaccess: use input constraints for ptr of __put_user
>   riscv: uaccess: use 'asm goto' for put_user()
>   riscv: uaccess: use 'asm goto output' for get_user
>
>  arch/riscv/include/asm/uaccess.h | 201 +++++++++++++++++++++++--------
>  1 file changed, 149 insertions(+), 52 deletions(-)

This genreally looks good to me, but there's some failures reported by 
the LKP tester and I don't think they're spurious.