riscv: uaccess: optimizations
Subsystems (5)
cover riscv: uaccess: optimizations
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(-)
Signature · 1 line
2.43.0
PATCH 1/4 riscv: implement user_access_begin and families
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.
--- arch/riscv/include/asm/uaccess.h | 63 ++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)
arch/riscv/include/asm/uaccess.h +63 -0
@@ -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 */
Signature · 1 line
2.43.0
Re: [PATCH 1/4] riscv: implement user_access_begin and families
Quoted 9 lines · 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_beginand families, the protection is disabled at the beginning of the copy and enabled at the end.The
__inttypemacro 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.
Quoted 86 lines
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/uaccess.h | 63 ++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)arch/riscv/include/asm/uaccess.h +63
-0@@ -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 */
PATCH 2/4 riscv: uaccess: use input constraints for ptr of __put_user
In reply to Jisheng ZhangI 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.
--- arch/riscv/include/asm/uaccess.h | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
arch/riscv/include/asm/uaccess.h +9 -9
@@ -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 */
Signature · 1 line
2.43.0
Arnd Bergmann
maintainerRe: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Quoted 6 lines · 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 <[email protected]>
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.
Quoted 7 lines
index
09d4ca37522c..84b084e388a7100644arch/riscv/include/asm/uaccess.h +0
-0@@ -186,11 +186,11 @@ do { \ __typeof__(*(ptr)) __x = x; \ __asm__ __volatile__ ( \ "1:\n" \
arch/riscv/include/asm/uaccess.h do { \ · line 195
Quoted 10 lines
arch/riscv/include/asm/uaccess.h +3
-3-" " 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 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Quoted 11 lines · Arnd Bergmann
Quoted 6 lines · 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 <[email protected]>I think this is a bit too confusing: clearly there is no read access from the
__userpointer, 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!
Quoted 16 lines
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.Quoted 12 lines
index
09d4ca37522c..84b084e388a7100644arch/riscv/include/asm/uaccess.h +1
-2@@ -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)) \
Quoted 8 lines
arch/riscv/include/asm/uaccess.h do { \ · line 195
Quoted 5 lines
arch/riscv/include/asm/uaccess.h +2
-1- : "rJ" (__x)); \ + : "+r" (err) \ + : "rJ" (__x), "m"(*(ptr))); \ }while (0 )I suspect this could just be a "r" constraint instead of "m", treating the
__userpointer 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.
Quoted 10 lines
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
__userpointers, 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_userside.Arnd
Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Quoted 43 lines · Jisheng Zhang
Quoted 11 lines · Arnd Bergmann
Quoted 6 lines · 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 <[email protected]>I think this is a bit too confusing: clearly there is no read access from the
__userpointer, so what you add in here is not correct. There also needs to be a code comment aboutHere is my understanding: the
__put_useris implemented withsd(or its less wider variant, sw etc.), w/o considering theex_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_userpiece 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.
*/
Quoted 6 lines
#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.
Quoted 71 lines
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!
Quoted 16 lines
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.Quoted 12 lines
index
09d4ca37522c..84b084e388a7100644arch/riscv/include/asm/uaccess.h +1
-2@@ -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)) \Quoted 8 lines
arch/riscv/include/asm/uaccess.h do { \ · line 195
Quoted 5 lines
arch/riscv/include/asm/uaccess.h +2
-1- : "rJ" (__x)); \ + : "+r" (err) \ + : "rJ" (__x), "m"(*(ptr))); \ }while (0 )I suspect this could just be a "r" constraint instead of "m", treating the
__userpointer 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.
Quoted 10 lines
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
__userpointers, 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_userside.Arnd
Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Quoted 61 lines · Jisheng Zhang
Quoted 43 lines · Jisheng Zhang
Quoted 11 lines · Arnd Bergmann
Quoted 6 lines · 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 <[email protected]>I think this is a bit too confusing: clearly there is no read access from the
__userpointer, so what you add in here is not correct. There also needs to be a code comment aboutHere is my understanding: the
__put_useris implemented withsd(or its less wider variant, sw etc.), w/o considering theex_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_userpiece 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.
*/Quoted 6 lines
#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_userpiece 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))
Quoted 1 line
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.
Quoted 73 lines
gcc to read the store address into one proper GP reg.
Quoted 71 lines
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!
Quoted 16 lines
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.Quoted 12 lines
index
09d4ca37522c..84b084e388a7100644arch/riscv/include/asm/uaccess.h +1
-2@@ -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)) \Quoted 8 lines
arch/riscv/include/asm/uaccess.h do { \ · line 195
Quoted 5 lines
arch/riscv/include/asm/uaccess.h +2
-1- : "rJ" (__x)); \ + : "+r" (err) \ + : "rJ" (__x), "m"(*(ptr))); \ }while (0 )I suspect this could just be a "r" constraint instead of "m", treating the
__userpointer 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.
Quoted 10 lines
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
__userpointers, 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_userside.Arnd
Re: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user
In reply to Jisheng ZhangQuoted 2 lines · Jisheng Zhang
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.
Signature · 3 lines
Andreas Schwab, SUSE Labs, [email protected] 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
Quoted 9 lines · Andreas Schwab
Quoted 2 lines · Jisheng Zhang
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
Quoted 5 lines
Signature · 4 lines
-- Andreas Schwab, SUSE Labs, [email protected] 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
In reply to Jisheng ZhangQuoted 3 lines · 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.
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).
Signature · 3 lines
Andreas Schwab, SUSE Labs, [email protected] 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
Quoted 9 lines · Andreas Schwab
Quoted 3 lines · 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.
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
Arnd Bergmann
maintainerRe: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Quoted 13 lines · Jisheng Zhang
Quoted 9 lines · Andreas Schwab
Quoted 3 lines · 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.
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,
Quoted 8 lines
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
Quoted 34 lines · Arnd Bergmann
Quoted 13 lines · Jisheng Zhang
Quoted 9 lines · Andreas Schwab
Quoted 3 lines · 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.
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,
Quoted 8 lines
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_64codebase 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))
Quoted 7 lines
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_structI 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!
Quoted 3 lines
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
Quoted 1 line
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?
Quoted 20 lines
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'.
Quoted 8 lines
For
put_user(), this cannot happen because the compiler doesn't know anything about the contents of the__userpointer. 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.
Arnd Bergmann
maintainerRe: [PATCH 2/4] riscv: uaccess: use input constraints for ptr of __put_user
Quoted 7 lines · Jisheng Zhang
"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.
Quoted 3 lines
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
In reply to Arnd BergmannFrom: Arnd Bergmann
Quoted 1 line
Sent: 26 June 2024 15:25
...
Quoted 3 lines
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)
PATCH 3/4 riscv: uaccess: use 'asm goto' for put_user()
In reply to 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.
--- arch/riscv/include/asm/uaccess.h | 68 +++++++++++++++----------------- 1 file changed, 32 insertions(+), 36 deletions(-)
arch/riscv/include/asm/uaccess.h +32 -36
@@ -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 *ptstatic 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; \
Signature · 1 line
2.43.0
kernel test robot
build botRe: [PATCH 3/4] riscv: uaccess: use 'asm goto' for put_user()
kernel test robot noticed the following build errors:
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/[email protected]/config) compiler: riscv32-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20240705/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
kernel test robot
build botRe: [PATCH 3/4] riscv: uaccess: use 'asm goto' for put_user()
In reply to Jisheng Zhangkernel test robot noticed the following build errors:
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/[email protected]/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/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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 Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
PATCH 4/4 riscv: uaccess: use 'asm goto output' for get_user
In reply to 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.
--- arch/riscv/include/asm/uaccess.h | 88 +++++++++++++++++++++++--------- 1 file changed, 63 insertions(+), 25 deletions(-)
arch/riscv/include/asm/uaccess.h +63 -25
@@ -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)/*
Signature · 1 line
2.43.0
kernel test robot
build botRe: [PATCH 4/4] riscv: uaccess: use 'asm goto output' for get_user
kernel test robot noticed the following build errors:
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/[email protected]/config) compiler: riscv32-linux-gcc (GCC) 13.2.0 reproduce: (https://download.01.org/0day-ci/archive/20240705/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Arnd Bergmann
maintainerQuoted 10 lines · Jisheng Zhang
This series tries to optimize riscv uaccess in the following way:
patch1 implement the
user_access_beginand families, I.E the unsafe accessors. when a function likestrncpy_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
Quoted 7 lines
patch3 uses 'asm goto' for
put_user()
patch4 uses 'asm goto output' forget_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
Quoted 6 lines · Arnd Bergmann
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.
Quoted 1 line
- __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
Quoted 32 lines · Linus Torvalds
Quoted 6 lines · Arnd Bergmann
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.
Quoted 1 line
- __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.
Quoted 9 lines
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.
Quoted 3 lines
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?
Quoted 4 lines · Linus Torvalds
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
clacwhich 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
retbecause 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
clacby 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
From 6b2c9a69efc21b9e6e0497a5661273f6fbe204b2 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <[email protected]>
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
clacwhere 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.
--- 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(-)
arch/x86/include/asm/uaccess_64.h +8 -0
@@ -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.
fs/select.c +3 -1
@@ -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);
include/linux/uaccess.h +7 -0
@@ -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
lib/strncpy_from_user.c +9 -0
@@ -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)) {
lib/strnlen_user.c +9 -0
@@ -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)) {
Quoted 9 lines · Linus Torvalds
Quoted 4 lines · Linus Torvalds
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.
[...]
Quoted 8 lines
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 theaccess_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
Quoted 4 lines · Will Deacon
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
Quoted 16 lines · Linus Torvalds
Quoted 4 lines · Will Deacon
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
Quoted 4 lines
- 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.
Quoted 6 lines
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.
Quoted 8 lines
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
Quoted 13 lines · Will Deacon
Quoted 12 lines · Linus Torvalds
On Fri, 5 Jul 2024 at 04:25, Will Deacon <[email protected]> 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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 3 lines
and I think it would resolve any issues we may have with wrapping. It still means effectively reverting
2305b809be93("arm64: uaccess:
simplifyuaccess_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).
Quoted 1 line
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.
Quoted 25 lines · Mark Rutland
Quoted 13 lines · Will Deacon
Quoted 12 lines · Linus Torvalds
On Fri, 5 Jul 2024 at 04:25, Will Deacon <[email protected]> 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.
Quoted 7 lines
Quoted 3 lines
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_D128where (1UL << 55) will be the start of the TTBR1 range in some configurations.
Quoted 7 lines
Quoted 3 lines
and I think it would resolve any issues we may have with wrapping. It still means effectively reverting
2305b809be93("arm64: uaccess:
simplifyuaccess_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
Quoted 8 lines · Linus Torvalds
Quoted 4 lines · Will Deacon
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.
Quoted 7 lines
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.
Quoted 4 lines
- 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.
Quoted 6 lines
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.
Quoted 8 lines
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.
Palmer Dabbelt
maintainerQuoted 19 lines · On Mon, 24 Jun 2024 21:04:56 PDT (-0700), [email protected] wrote:
This series tries to optimize riscv uaccess in the following way:
patch1 implement the
user_access_beginand families, I.E the unsafe accessors. when a function likestrncpy_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' forget_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.
Quoted 8 lines
Jisheng Zhang (4):
riscv: implementuser_access_beginand families
riscv: uaccess: use input constraints for ptr of__put_user
riscv: uaccess: use 'asm goto' forput_user()
riscv: uaccess: use 'asm goto output' forget_userarch/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.