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

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

[Cyril Bur]

Orignally sent by Jisheng Zhang <jszhang@kernel.org>

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.

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.
I did read the discussion this series generated. It isn't clear to me
which direction to take the patches, if any.

[PATCH v2 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 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.34.1

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

[Charlie Jenkins]

On Mon, Nov 18, 2024 at 11:01:09PM +0000, 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 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)

Since a handful of these functions are duplicated across x86/arm64 it
would be nice to consolidate them to a shared header. That would
probably require quite a bit more work though so not in scope for this
patch.

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.com>

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

[Ben Dooks]

On 17/01/2025 23:21, Charlie Jenkins wrote:
> On Mon, Nov 18, 2024 at 11:01:09PM +0000, 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>

If we're doing this, then saving the STATUS.SUM flag is going to
be more important than before. We had this discussion when the
initial user-access with syzbot stress testing turned up.

We partially fixed this by rewriting the ordering in the __put_user
function to stop the 'x' argument being evaluated inside the area
where SUM is enabled, but this is going to make the window of
opportunity of a thread switch much bigger and the bug will just
come back and bite harder.

If you want I can look at re-doing my original patch and resubmitting.


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

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

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

[Cyril Bur]

On 6/2/2025 1:08 am, Ben Dooks wrote:
> On 17/01/2025 23:21, Charlie Jenkins wrote:
>> On Mon, Nov 18, 2024 at 11:01:09PM +0000, 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>
> 
> If we're doing this, then saving the STATUS.SUM flag is going to
> be more important than before. We had this discussion when the
> initial user-access with syzbot stress testing turned up.
> 
> We partially fixed this by rewriting the ordering in the __put_user
> function to stop the 'x' argument being evaluated inside the area
> where SUM is enabled, but this is going to make the window of
> opportunity of a thread switch much bigger and the bug will just
> come back and bite harder.
> 
> If you want I can look at re-doing my original patch and resubmitting.

Oh! Could you please link the patch? I haven't seen it and can't seem to 
find it now.

Thanks.

> 
> 

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

[Ben Dooks]

On 13/02/2025 17:07, Cyril Bur wrote:
> 
> 
> On 6/2/2025 1:08 am, Ben Dooks wrote:
>> On 17/01/2025 23:21, Charlie Jenkins wrote:
>>> On Mon, Nov 18, 2024 at 11:01:09PM +0000, 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>
>>
>> If we're doing this, then saving the STATUS.SUM flag is going to
>> be more important than before. We had this discussion when the
>> initial user-access with syzbot stress testing turned up.
>>
>> We partially fixed this by rewriting the ordering in the __put_user
>> function to stop the 'x' argument being evaluated inside the area
>> where SUM is enabled, but this is going to make the window of
>> opportunity of a thread switch much bigger and the bug will just
>> come back and bite harder.
>>
>> If you want I can look at re-doing my original patch and resubmitting.
> 
> Oh! Could you please link the patch? I haven't seen it and can't seem to 
> find it now.

https://lore.kernel.org/linux-riscv/20210318151010.100966-1-ben.dooks@codethink.co.uk/

> Thanks.
> 
>>
>>
> 
> 


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

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

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

[Cyril Bur]

On 14/2/2025 3:16 am, Ben Dooks wrote:
> On 13/02/2025 17:07, Cyril Bur wrote:
>>
>>
>> On 6/2/2025 1:08 am, Ben Dooks wrote:
>>> On 17/01/2025 23:21, Charlie Jenkins wrote:
>>>> On Mon, Nov 18, 2024 at 11:01:09PM +0000, 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>
>>>
>>> If we're doing this, then saving the STATUS.SUM flag is going to
>>> be more important than before. We had this discussion when the
>>> initial user-access with syzbot stress testing turned up.
>>>
>>> We partially fixed this by rewriting the ordering in the __put_user
>>> function to stop the 'x' argument being evaluated inside the area
>>> where SUM is enabled, but this is going to make the window of
>>> opportunity of a thread switch much bigger and the bug will just
>>> come back and bite harder.
>>>
>>> If you want I can look at re-doing my original patch and resubmitting.
>>
>> Oh! Could you please link the patch? I haven't seen it and can't seem 
>> to find it now.
> 
> https://lore.kernel.org/linux-riscv/20210318151010.100966-1- 
> ben.dooks@codethink.co.uk/

I agree we want this patch. Or at least we want clarity around calling 
schedule inside SUM enabled sections.

Other arches are stricter about not calling schedule at all. I'm not 
really going to advocate for one way or the other right now but I 
believe your patch would solve the problem.

Cyril

> 
>> Thanks.
>>
>>>
>>>
>>
>>
> 
> 

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

[Cyril Bur]

From: Jisheng Zhang <jszhang@kernel.org>

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>
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 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.34.1

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

[Charlie Jenkins]

On Mon, Nov 18, 2024 at 11:01:10PM +0000, Cyril Bur wrote:
> From: Jisheng Zhang <jszhang@kernel.org>
> 
> 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>
> Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.com>

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

[Jessica Clarke]

On 18 Nov 2024, at 23:01, Cyril Bur <cyrilbur@tenstorrent.com> wrote:
> 
> From: Jisheng Zhang <jszhang@kernel.org>
> 
> 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.

That’s not what =m does. =m tells the compiler “this assembly will be
writing to this memory location, and needs the address of the location
for that operand”. If you move it from an output to an input then you
get the same assembly generated, but the compiler believes you are
*reading* from that memory, not *writing* to it, and so does not
believe the memory may be clobbered.

Now, it may well be that, by virtue of this being userspace memory that
is only ever accessed via assembly, and any inline assembly being
marked volatile, this in effect doesn’t matter. But it is still
technically wrong to model it that way, and you cannot use the
justification you are using, because it is false, and demonstrates a
lack of understanding for how inline assembly works. It has to be
correctly justified.

(I do note that neither x86 nor arm64 seems to model this as an output)

Jess

> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> 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 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.34.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Cyril Bur]

On 18/1/2025 9:34 am, Jessica Clarke wrote:
> On 18 Nov 2024, at 23:01, Cyril Bur <cyrilbur@tenstorrent.com> wrote:
>>
>> From: Jisheng Zhang <jszhang@kernel.org>
>>
>> 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.
> 
> That’s not what =m does. =m tells the compiler “this assembly will be
> writing to this memory location, and needs the address of the location
> for that operand”. If you move it from an output to an input then you
> get the same assembly generated, but the compiler believes you are
> *reading* from that memory, not *writing* to it, and so does not
> believe the memory may be clobbered.
> 
> Now, it may well be that, by virtue of this being userspace memory that
> is only ever accessed via assembly, and any inline assembly being
> marked volatile, this in effect doesn’t matter. But it is still
> technically wrong to model it that way, and you cannot use the
> justification you are using, because it is false, and demonstrates a
> lack of understanding for how inline assembly works. It has to be
> correctly justified.
> 
> (I do note that neither x86 nor arm64 seems to model this as an output)
> 
> Jess

Good points, I agree. I am happy to respin this patch move the =m to 
output constraint.

@Charlie Please let me know if you would like me to keep your tags on 
the tweaked version.

Thanks,

Cyril
> 
>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>> 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 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.34.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

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

[Charlie Jenkins]

On Fri, Jan 31, 2025 at 12:31:38PM +1100, Cyril Bur wrote:
> 
> 
> On 18/1/2025 9:34 am, Jessica Clarke wrote:
> > On 18 Nov 2024, at 23:01, Cyril Bur <cyrilbur@tenstorrent.com> wrote:
> > > 
> > > From: Jisheng Zhang <jszhang@kernel.org>
> > > 
> > > 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.
> > 
> > That’s not what =m does. =m tells the compiler “this assembly will be
> > writing to this memory location, and needs the address of the location
> > for that operand”. If you move it from an output to an input then you
> > get the same assembly generated, but the compiler believes you are
> > *reading* from that memory, not *writing* to it, and so does not
> > believe the memory may be clobbered.
> > 
> > Now, it may well be that, by virtue of this being userspace memory that
> > is only ever accessed via assembly, and any inline assembly being
> > marked volatile, this in effect doesn’t matter. But it is still
> > technically wrong to model it that way, and you cannot use the
> > justification you are using, because it is false, and demonstrates a
> > lack of understanding for how inline assembly works. It has to be
> > correctly justified.
> > 
> > (I do note that neither x86 nor arm64 seems to model this as an output)
> > 
> > Jess
> 
> Good points, I agree. I am happy to respin this patch
> move the =m to output constraint.
> 
> @Charlie Please let me know if you would like me to
> keep your tags on the tweaked version.

Can you remove the tags? Copy me on the next version and I will review
it again.

- Charlie

> 
> Thanks,
> 
> Cyril
> > 
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > 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 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.34.1
> > > 
> > > 
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > 
> 

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

[Cyril Bur]

From: Jisheng Zhang <jszhang@kernel.org>

'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>
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 84b084e388a7..916df594ef3c 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__ (					\
+	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.
@@ -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.34.1

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

[Charlie Jenkins]

On Mon, Nov 18, 2024 at 11:01:11PM +0000, Cyril Bur wrote:
> From: Jisheng Zhang <jszhang@kernel.org>
> 
> '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>
> Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.com>

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

[Cyril Bur]

From: Jisheng Zhang <jszhang@kernel.org>

'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>
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 916df594ef3c..62a2ddf8c542 100644
--- a/arch/riscv/include/asm/uaccess.h
+++ b/arch/riscv/include/asm/uaccess.h
@@ -63,27 +63,56 @@
  * 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"				\
@@ -92,35 +121,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 +190,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 +384,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 +406,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.34.1

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

[Charlie Jenkins]

On Mon, Nov 18, 2024 at 11:01:12PM +0000, Cyril Bur wrote:
> From: Jisheng Zhang <jszhang@kernel.org>
> 
> '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>
> Signed-off-by: Cyril Bur <cyrilbur@tenstorrent.com>

Reviewed-by: Charlie Jenkins <charlie@rivosinc.com>
Tested-by: Charlie Jenkins <charlie@rivosinc.com>