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

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

[Cyril Bur]

This series tries to optimize riscv uaccess by allowing the use of
user_access_begin() and user_access_end() which permits grouping user accesses
and avoiding the CSR write penalty for each access.

The error path can also be optimised using asm goto which patches 3 and 4
achieve. This will speed up jumping to labels by avoiding the need of an
intermediary error type variable within the uaccess macros

I did read the discussion this series generated. It isn't clear to me
which direction to take the patches, if any.

V2:
I've taken on this series as there isn't any response from Jisheng. No
significant changes other than build fixes.
- Fixes build breakage in patch 3 to do with not having used 'goto' keyword.
- Fixes build breakage in patch 4 on 32bit not having delcared __ptr in the
  macro.

V3:
Significant commit message rewrites.
 - Corrected the justification for patch 2
 - Better explained/justified patches 3 and 4
Minor code changes for legibility and more comments.

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 | 205 +++++++++++++++++++++++--------
 1 file changed, 152 insertions(+), 53 deletions(-)

-- 
2.34.1


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

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

[Cyril Bur]

From: Jisheng Zhang <jszhang@kernel.org>

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>
Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
---
 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 fee56b0c8058..43db1d9c2f99 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -61,6 +61,19 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, unsigne
 #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
@@ -368,6 +381,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) { }
+
+/*
+ * We want the unsafe accessors to always be inlined and use
+ * the error labels - thus the macro games.
+ */
+#define unsafe_put_user(x, ptr, label)	do {				\
+	long __err = 0;							\
+	__put_user_nocheck(x, (ptr), __err);				\
+	if (__err) goto label;						\
+} while (0)
+
+#define unsafe_get_user(x, ptr, label)	do {				\
+	long __err = 0;							\
+	__inttype(*(ptr)) __gu_val;					\
+	__get_user_nocheck(__gu_val, (ptr), __err);			\
+	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
+	if (__err) goto label;						\
+} while (0)
+
+#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.34.1


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

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

[Cyril Bur]

From: Jisheng Zhang <jszhang@kernel.org>

Putting ptr in the inputs as opposed to output may seem incorrect but
this is done for a few reasons:
- Not having it in the output permits the use of asm goto in a
  subsequent patch. There are bugs in gcc [1] which would otherwise
  prevent it.
- Since the output memory is userspace there isn't any real benefit from
  telling the compiler about the memory clobber.
- x86, arm and powerpc all use this technique.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921 # 1

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
[Cyril Bur: Rewritten commit message]
Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
---
 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 43db1d9c2f99..fb3961ecf38b 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -219,11 +219,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
@@ -236,16 +236,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.34.1


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

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

[Cyril Bur]

From: Jisheng Zhang <jszhang@kernel.org>

With 'asm goto' we don't need to test the error etc, the exception just
jumps to the error handling directly.

Because there are no output clobbers which could trigger gcc bugs [1]
the use of asm_goto_output() macro is not necessary here. Not using
asm_goto_output() is desirable as the generated output asm will be
cleaner.

Use of the volatile keyword is redundant as per gcc 14.2.0 manual section
6.48.2.7 Goto Labels:
> Also note that an asm goto statement is always implicitly considered
  volatile.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921 # 1

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
[Cyril Bur: Rewritten commit message]
Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
---
 arch/riscv/include/asm/uaccess.h | 70 +++++++++++++++-----------------
 1 file changed, 33 insertions(+), 37 deletions(-)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index fb3961ecf38b..1b926b61af66 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -214,61 +214,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__ (					\
+	asm goto(						\
 		"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.
@@ -299,7 +304,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;						\
@@ -373,13 +378,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)
 {
@@ -398,11 +397,8 @@ static inline void user_access_restore(unsigned long enabled) { }
  * We want the unsafe accessors to always be inlined and use
  * the error labels - thus the macro games.
  */
-#define unsafe_put_user(x, ptr, label)	do {				\
-	long __err = 0;							\
-	__put_user_nocheck(x, (ptr), __err);				\
-	if (__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 __err = 0;							\
-- 
2.34.1


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

[PATCH v3 4/4] riscv: uaccess: use 'asm_goto_output' for get_user()

[Cyril Bur]

From: Jisheng Zhang <jszhang@kernel.org>

With 'asm goto' we don't need to test the error etc, the exception just
jumps to the error handling directly.

Unlike put_user(), get_user() must work around GCC bugs [1] when using
output clobbers in an asm goto statement.

Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921 # 1

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
[Cyril Bur: Rewritten commit message]
Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>
---
 arch/riscv/include/asm/uaccess.h | 90 +++++++++++++++++++++++---------
 1 file changed, 65 insertions(+), 25 deletions(-)

diff --git a/arch/riscv/include/asm/uaccess.h b/arch/riscv/include/asm/uaccess.h
index 1b926b61af66..ab48ef57565e 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -96,27 +96,56 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, unsigne
  * 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)				\
+	u32 __user *__ptr = (u32 __user *)(ptr);		\
+	u32 __lo, __hi;						\
+	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"				\
@@ -125,35 +154,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.
@@ -178,13 +223,16 @@ do {								\
 ({								\
 	const __typeof__(*(ptr)) __user *__gu_ptr = untagged_addr(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;						\
 })
 
@@ -369,13 +417,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)
@@ -401,11 +443,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 __err = 0;							\
 	__inttype(*(ptr)) __gu_val;					\
-	__get_user_nocheck(__gu_val, (ptr), __err);			\
+	__get_user_nocheck(__gu_val, (ptr), label);			\
 	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
-	if (__err) goto label;						\
 } while (0)
 
 #define unsafe_copy_loop(dst, src, len, type, label)				\
-- 
2.34.1


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

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

[Anton Blanchard]

Hi Cyril,

On Fri, Feb 21, 2025 at 11:09 AM Cyril Bur <cyrilbur@tenstorrent.com> wrote:
> This series tries to optimize riscv uaccess by allowing the use of
> user_access_begin() and user_access_end() which permits grouping user accesses
> and avoiding the CSR write penalty for each access.

I tested this on the upcoming Tenstorrent Ascalon CPU using a simple
microbenchmark (getdents64() on a directory with 10 files in it) and it was
significantly faster (over 40%). This came from both a reduction in
instruction count as well as a reduction in sstatus CSR writes.

It would be great if we could get this merged considering x86, arm64 and
POWER are exploiting this optimisation already.

Thanks,
Anton

> The error path can also be optimised using asm goto which patches 3 and 4
> achieve. This will speed up jumping to labels by avoiding the need of an
> intermediary error type variable within the uaccess macros
>
> I did read the discussion this series generated. It isn't clear to me
> which direction to take the patches, if any.
>
> V2:
> I've taken on this series as there isn't any response from Jisheng. No
> significant changes other than build fixes.
> - Fixes build breakage in patch 3 to do with not having used 'goto' keyword.
> - Fixes build breakage in patch 4 on 32bit not having delcared __ptr in the
>   macro.
>
> V3:
> Significant commit message rewrites.
>  - Corrected the justification for patch 2
>  - Better explained/justified patches 3 and 4
> Minor code changes for legibility and more comments.
>
> 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 | 205 +++++++++++++++++++++++--------
>  1 file changed, 152 insertions(+), 53 deletions(-)
>
> --
> 2.34.1
>
>

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

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

[Alexandre Ghiti]

Hi Cyril,

On 21/02/2025 01:09, Cyril Bur wrote:
> This series tries to optimize riscv uaccess by allowing the use of
> user_access_begin() and user_access_end() which permits grouping user accesses
> and avoiding the CSR write penalty for each access.
>
> The error path can also be optimised using asm goto which patches 3 and 4
> achieve. This will speed up jumping to labels by avoiding the need of an
> intermediary error type variable within the uaccess macros
>
> I did read the discussion this series generated. It isn't clear to me
> which direction to take the patches, if any.
>
> V2:
> I've taken on this series as there isn't any response from Jisheng. No
> significant changes other than build fixes.
> - Fixes build breakage in patch 3 to do with not having used 'goto' keyword.
> - Fixes build breakage in patch 4 on 32bit not having delcared __ptr in the
>    macro.
>
> V3:
> Significant commit message rewrites.
>   - Corrected the justification for patch 2
>   - Better explained/justified patches 3 and 4
> Minor code changes for legibility and more comments.
>
> 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 | 205 +++++++++++++++++++++++--------
>   1 file changed, 152 insertions(+), 53 deletions(-)
>
Following up on Ben's comment here 
https://lore.kernel.org/linux-riscv/b45aab1e-6d37-4027-9a15-4fa917d806b9@codethink.co.uk/

The problem that Ben mentions is caused by the use of *macros* which 
used to make the evaluation of the parameter inside the user-accessible 
section, and since this parameter could be a sleeping function, we could 
schedule another process with the SUM bit set, which could be cleared by 
this process, which would make the first process fault when trying to 
access user memory. I did not find any macro using unsafe_XXX() 
functions which could cause a problem right now, but I may have missed 
one and new could come up later, so we have multiple solutions here:

- suppose that a macro using unsafe_get/put_user() and passing a 
sleeping function as argument won't happen and then do nothing
- or save/restore CSR sstatus when switching processes
- or simply check that SUM is not set when switching processes

Let me know what you think.

Thanks,

Alex


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

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

[Alexandre Ghiti]

Hi Cyril,

On 21/02/2025 01:09, Cyril Bur wrote:
> From: Jisheng Zhang<jszhang@kernel.org>
>
> 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>
> Signed-off-by: Cyril Bur<cyrilbur@tenstorrent.com>
> ---
>   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 fee56b0c8058..43db1d9c2f99 100644
> --- a/arch/riscv/include/asm/uaccess.h
> +++ b/arch/riscv/include/asm/uaccess.h
> @@ -61,6 +61,19 @@ static inline unsigned long __untagged_addr_remote(struct mm_struct *mm, unsigne
>   #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
> @@ -368,6 +381,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)


Nit: no need for (a,b) here


> +#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) { }
> +
> +/*
> + * We want the unsafe accessors to always be inlined and use
> + * the error labels - thus the macro games.
> + */
> +#define unsafe_put_user(x, ptr, label)	do {				\
> +	long __err = 0;							\
> +	__put_user_nocheck(x, (ptr), __err);				\
> +	if (__err) goto label;						\
> +} while (0)
> +
> +#define unsafe_get_user(x, ptr, label)	do {				\
> +	long __err = 0;							\
> +	__inttype(*(ptr)) __gu_val;					\
> +	__get_user_nocheck(__gu_val, (ptr), __err);			\
> +	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
> +	if (__err) goto label;						\
> +} while (0)
> +
> +#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 */

There is a bunch of checkpatch errors to fix, see 
https://gist.github.com/linux-riscv-bot/98f23fd1b04d6da7c23c6cb18245a158

Why isn't there an implementation for unsafe_copy_from_user()? Let's 
take the following example:

user_access_begin()
unsafe_copy_from_user()
unsafe_get_user() <==== This one will fail since unsafe_copy_from_user() 
-> raw_copy_from_user() -> __asm_vector_usercopy() which enables and 
disables the SUM bit.
user_access_end()

Another thing is that with this patch, we lose the vectorized user 
access functions, can you fix that too?

Thanks,

Alex




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

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

[Ben Dooks]

On 14/03/2025 13:28, Alexandre Ghiti wrote:
> Hi Cyril,
> 
> On 21/02/2025 01:09, Cyril Bur wrote:
>> This series tries to optimize riscv uaccess by allowing the use of
>> user_access_begin() and user_access_end() which permits grouping user 
>> accesses
>> and avoiding the CSR write penalty for each access.
>>
>> The error path can also be optimised using asm goto which patches 3 and 4
>> achieve. This will speed up jumping to labels by avoiding the need of an
>> intermediary error type variable within the uaccess macros
>>
>> I did read the discussion this series generated. It isn't clear to me
>> which direction to take the patches, if any.
>>
>> V2:
>> I've taken on this series as there isn't any response from Jisheng. No
>> significant changes other than build fixes.
>> - Fixes build breakage in patch 3 to do with not having used 'goto' 
>> keyword.
>> - Fixes build breakage in patch 4 on 32bit not having delcared __ptr 
>> in the
>>    macro.
>>
>> V3:
>> Significant commit message rewrites.
>>   - Corrected the justification for patch 2
>>   - Better explained/justified patches 3 and 4
>> Minor code changes for legibility and more comments.
>>
>> 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 | 205 +++++++++++++++++++++++--------
>>   1 file changed, 152 insertions(+), 53 deletions(-)
>>
> Following up on Ben's comment here https://lore.kernel.org/linux-riscv/ 
> b45aab1e-6d37-4027-9a15-4fa917d806b9@codethink.co.uk/
> 
> The problem that Ben mentions is caused by the use of *macros* which 
> used to make the evaluation of the parameter inside the user-accessible 
> section, and since this parameter could be a sleeping function, we could 
> schedule another process with the SUM bit set, which could be cleared by 
> this process, which would make the first process fault when trying to 
> access user memory. I did not find any macro using unsafe_XXX() 
> functions which could cause a problem right now, but I may have missed 
> one and new could come up later, so we have multiple solutions here:
> 
> - suppose that a macro using unsafe_get/put_user() and passing a 
> sleeping function as argument won't happen and then do nothing
> - or save/restore CSR sstatus when switching processes
> - or simply check that SUM is not set when switching processes
> 
> Let me know what you think.

I'm on the save the flag side, for these reasons:

#1 sleeping functions can happen more often when various checks
    get enabled in the kernel (this was why the original fault
    was found).  Adding larger sections is just going to make
    the fault pop up again at some point in the future.

#2 the save/restore is a small addition to the swap registers

#3 saving SUM over a regs swap is always going to make sure we
    never see this gremlin turn up again

FYI, I think I may have posted our original test thread at some
point, but I could do so again.

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


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

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

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

[Cyril Bur]

On 15/3/2025 12:49 am, Ben Dooks wrote:
> On 14/03/2025 13:28, Alexandre Ghiti wrote:
>> Hi Cyril,
>>
>> On 21/02/2025 01:09, Cyril Bur wrote:
>>> This series tries to optimize riscv uaccess by allowing the use of
>>> user_access_begin() and user_access_end() which permits grouping user 
>>> accesses
>>> and avoiding the CSR write penalty for each access.
>>>
>>> The error path can also be optimised using asm goto which patches 3 
>>> and 4
>>> achieve. This will speed up jumping to labels by avoiding the need of an
>>> intermediary error type variable within the uaccess macros
>>>
>>> I did read the discussion this series generated. It isn't clear to me
>>> which direction to take the patches, if any.
>>>
>>> V2:
>>> I've taken on this series as there isn't any response from Jisheng. No
>>> significant changes other than build fixes.
>>> - Fixes build breakage in patch 3 to do with not having used 'goto' 
>>> keyword.
>>> - Fixes build breakage in patch 4 on 32bit not having delcared __ptr 
>>> in the
>>>    macro.
>>>
>>> V3:
>>> Significant commit message rewrites.
>>>   - Corrected the justification for patch 2
>>>   - Better explained/justified patches 3 and 4
>>> Minor code changes for legibility and more comments.
>>>
>>> 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 | 205 +++++++++++++++++++++++--------
>>>   1 file changed, 152 insertions(+), 53 deletions(-)
>>>
>> Following up on Ben's comment here https://lore.kernel.org/linux- 
>> riscv/ b45aab1e-6d37-4027-9a15-4fa917d806b9@codethink.co.uk/
>>
>> The problem that Ben mentions is caused by the use of *macros* which 
>> used to make the evaluation of the parameter inside the user- 
>> accessible section, and since this parameter could be a sleeping 
>> function, we could schedule another process with the SUM bit set, 
>> which could be cleared by this process, which would make the first 
>> process fault when trying to access user memory. I did not find any 
>> macro using unsafe_XXX() functions which could cause a problem right 
>> now, but I may have missed one and new could come up later, so we have 
>> multiple solutions here:
>>
>> - suppose that a macro using unsafe_get/put_user() and passing a 
>> sleeping function as argument won't happen and then do nothing
>> - or save/restore CSR sstatus when switching processes
>> - or simply check that SUM is not set when switching processes
>>
>> Let me know what you think.
> 
> I'm on the save the flag side, for these reasons:
> 
> #1 sleeping functions can happen more often when various checks
>     get enabled in the kernel (this was why the original fault
>     was found).  Adding larger sections is just going to make
>     the fault pop up again at some point in the future.
> 
> #2 the save/restore is a small addition to the swap registers
> 
> #3 saving SUM over a regs swap is always going to make sure we
>     never see this gremlin turn up again
> 
> FYI, I think I may have posted our original test thread at some
> point, but I could do so again.

Yes, after Ben pointed out the issue I came to the conclusion we 
probably want Bens patch which saves the bit. Apologies if I didn't 
express this thought in email.

I'm happy to take the patch and put it on the front of this series, 
although perhaps it makes more sense you to revive the patch since 
you're still around Ben?

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


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

Re: [EXT] Re: [PATCH v3 1/4] riscv: implement user_access_begin() and families

[Cyril Bur]

On 15/3/2025 12:28 am, Alexandre Ghiti wrote:
> Hi Cyril,
> 
> On 21/02/2025 01:09, Cyril Bur wrote:
>> From: Jisheng Zhang<jszhang@kernel.org>
>>
>> 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>
>> Signed-off-by: Cyril Bur<cyrilbur@tenstorrent.com>
>> ---
>>   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 fee56b0c8058..43db1d9c2f99 100644
>> --- a/arch/riscv/include/asm/uaccess.h
>> +++ b/arch/riscv/include/asm/uaccess.h
>> @@ -61,6 +61,19 @@ static inline unsigned long 
>> __untagged_addr_remote(struct mm_struct *mm, unsigne
>>   #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
>> @@ -368,6 +381,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)
> 
> 
> Nit: no need for (a,b) here
> 
> 
>> +#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) { }
>> +
>> +/*
>> + * We want the unsafe accessors to always be inlined and use
>> + * the error labels - thus the macro games.
>> + */
>> +#define unsafe_put_user(x, ptr, label)    do {                \
>> +    long __err = 0;                            \
>> +    __put_user_nocheck(x, (ptr), __err);                \
>> +    if (__err) goto label;                        \
>> +} while (0)
>> +
>> +#define unsafe_get_user(x, ptr, label)    do {                \
>> +    long __err = 0;                            \
>> +    __inttype(*(ptr)) __gu_val;                    \
>> +    __get_user_nocheck(__gu_val, (ptr), __err);            \
>> +    (x) = (__force __typeof__(*(ptr)))__gu_val;            \
>> +    if (__err) goto label;                        \
>> +} while (0)
>> +
>> +#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 */
> 
> There is a bunch of checkpatch errors to fix, see https:// 
> gist.github.com/linux-riscv-bot/98f23fd1b04d6da7c23c6cb18245a158
> 

Oops, yeah will fix.

> Why isn't there an implementation for unsafe_copy_from_user()? Let's 
> take the following example:
> 
> user_access_begin()
> unsafe_copy_from_user()
> unsafe_get_user() <==== This one will fail since unsafe_copy_from_user() 
> -> raw_copy_from_user() -> __asm_vector_usercopy() which enables and 
> disables the SUM bit.
> user_access_end()

I'll have to look into that - thanks for the feedback.

> 
> Another thing is that with this patch, we lose the vectorized user 
> access functions, can you fix that too?

I'll have a look at this also.

> 
> Thanks,
> 
> Alex
> 
> 
> 


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

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

[Ben Dooks]

On 17/03/2025 23:52, Cyril Bur wrote:
> 
> 
> On 15/3/2025 12:49 am, Ben Dooks wrote:
>> On 14/03/2025 13:28, Alexandre Ghiti wrote:
>>> Hi Cyril,
>>>
>>> On 21/02/2025 01:09, Cyril Bur wrote:
>>>> This series tries to optimize riscv uaccess by allowing the use of
>>>> user_access_begin() and user_access_end() which permits grouping 
>>>> user accesses
>>>> and avoiding the CSR write penalty for each access.
>>>>
>>>> The error path can also be optimised using asm goto which patches 3 
>>>> and 4
>>>> achieve. This will speed up jumping to labels by avoiding the need 
>>>> of an
>>>> intermediary error type variable within the uaccess macros
>>>>
>>>> I did read the discussion this series generated. It isn't clear to me
>>>> which direction to take the patches, if any.
>>>>
>>>> V2:
>>>> I've taken on this series as there isn't any response from Jisheng. No
>>>> significant changes other than build fixes.
>>>> - Fixes build breakage in patch 3 to do with not having used 'goto' 
>>>> keyword.
>>>> - Fixes build breakage in patch 4 on 32bit not having delcared __ptr 
>>>> in the
>>>>    macro.
>>>>
>>>> V3:
>>>> Significant commit message rewrites.
>>>>   - Corrected the justification for patch 2
>>>>   - Better explained/justified patches 3 and 4
>>>> Minor code changes for legibility and more comments.
>>>>
>>>> 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 | 205 ++++++++++++++++++++++ 
>>>> +--------
>>>>   1 file changed, 152 insertions(+), 53 deletions(-)
>>>>
>>> Following up on Ben's comment here https://lore.kernel.org/linux- 
>>> riscv/ b45aab1e-6d37-4027-9a15-4fa917d806b9@codethink.co.uk/
>>>
>>> The problem that Ben mentions is caused by the use of *macros* which 
>>> used to make the evaluation of the parameter inside the user- 
>>> accessible section, and since this parameter could be a sleeping 
>>> function, we could schedule another process with the SUM bit set, 
>>> which could be cleared by this process, which would make the first 
>>> process fault when trying to access user memory. I did not find any 
>>> macro using unsafe_XXX() functions which could cause a problem right 
>>> now, but I may have missed one and new could come up later, so we 
>>> have multiple solutions here:
>>>
>>> - suppose that a macro using unsafe_get/put_user() and passing a 
>>> sleeping function as argument won't happen and then do nothing
>>> - or save/restore CSR sstatus when switching processes
>>> - or simply check that SUM is not set when switching processes
>>>
>>> Let me know what you think.
>>
>> I'm on the save the flag side, for these reasons:
>>
>> #1 sleeping functions can happen more often when various checks
>>     get enabled in the kernel (this was why the original fault
>>     was found).  Adding larger sections is just going to make
>>     the fault pop up again at some point in the future.
>>
>> #2 the save/restore is a small addition to the swap registers
>>
>> #3 saving SUM over a regs swap is always going to make sure we
>>     never see this gremlin turn up again
>>
>> FYI, I think I may have posted our original test thread at some
>> point, but I could do so again.
> 
> Yes, after Ben pointed out the issue I came to the conclusion we 
> probably want Bens patch which saves the bit. Apologies if I didn't 
> express this thought in email.
> 
> I'm happy to take the patch and put it on the front of this series, 
> although perhaps it makes more sense you to revive the patch since 
> you're still around Ben?

Yes, I'm currently very busy so happy for someone else to get this
merged and tested.

We could do with some better testing on whether we leak flags on
task switches (possibly), but that's a side quest and not for now.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

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

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

[Cyril Bur]

On 15/3/2025 12:28 am, Alexandre Ghiti wrote:
> Hi Cyril,
> 
> 
> There is a bunch of checkpatch errors to fix, see https:// 
> gist.github.com/linux-riscv-bot/98f23fd1b04d6da7c23c6cb18245a158
> 
> Why isn't there an implementation for unsafe_copy_from_user()? Let's 
> take the following example:
> 
> user_access_begin()
> unsafe_copy_from_user()
> unsafe_get_user() <==== This one will fail since unsafe_copy_from_user() 
> -> raw_copy_from_user() -> __asm_vector_usercopy() which enables and 
> disables the SUM bit.
> user_access_end()
> 
> Another thing is that with this patch, we lose the vectorized user 
> access functions, can you fix that too?

I've just been looking at doing that. I think teasing out an 'unsafe' 
style vectorised version and shoehorning it into this patch will be too 
much.

Would it be possible for me to do this as a separate series? I'm 
motivated to have this happen - we have a vector unit, we'll definitely 
want to use it.

I will add that a 'standard' copy_to_user() would still find its way to 
calling raw_copy_to_user() and so could be vectorised if all other 
criteria are met.

Thanks,

Cyril

> 
> Thanks,
> 
> Alex
> 
> 
> 


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