| Message ID | 32268431948dc1a32264a98a76d41d71ae7536b3.1521141122.git.msuchanek@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc barrier_nospec | expand |
On Thu, Mar 15, 2018 at 12:15 PM, Michal Suchanek <msuchanek@suse.de> wrote: > This is based on x86 patch doing the same. > > Signed-off-by: Michal Suchanek <msuchanek@suse.de> > --- > --- a/arch/powerpc/include/asm/uaccess.h > +++ b/arch/powerpc/include/asm/uaccess.h > @@ -258,8 +259,10 @@ do { \ > long __gu_err = -EFAULT; \ > unsigned long __gu_val = 0; \ > const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ > + int can_access = access_ok(VERIFY_READ, __gu_addr, (size)); \ > might_fault(); \ > - if (access_ok(VERIFY_READ, __gu_addr, (size))) \ > + barrier_nospec(); \ > + if (can_access) \ > __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ > (x) = (__force __typeof__(*(ptr)))__gu_val; \ > __gu_err; \ Is the above really correct? The barrier is *before* the conditional branch that might be mis-predicted. I don't know how the ppc barrier works, but that sounds completely bogus. Linus
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Thu, Mar 15, 2018 at 12:15 PM, Michal Suchanek <msuchanek@suse.de> wrote: >> This is based on x86 patch doing the same. >> >> Signed-off-by: Michal Suchanek <msuchanek@suse.de> >> --- >> --- a/arch/powerpc/include/asm/uaccess.h >> +++ b/arch/powerpc/include/asm/uaccess.h >> @@ -258,8 +259,10 @@ do { \ >> long __gu_err = -EFAULT; \ >> unsigned long __gu_val = 0; \ >> const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ >> + int can_access = access_ok(VERIFY_READ, __gu_addr, (size)); \ >> might_fault(); \ >> - if (access_ok(VERIFY_READ, __gu_addr, (size))) \ >> + barrier_nospec(); \ >> + if (can_access) \ >> __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ >> (x) = (__force __typeof__(*(ptr)))__gu_val; \ >> __gu_err; \ > > Is the above really correct? The barrier is *before* the conditional > branch that might be mis-predicted. > > I don't know how the ppc barrier works, but that sounds completely bogus. Yeah it should be after the branch. I don't have a formal spec for the barrier yet, it should be defined in a hopefully soon to be released revision of the ISA. But the gist is it will stall execution until any older branches are no longer speculating. It doesn't order any two arbitrary instructions, such as a comparison and a branch, which I suspect is how Michal was interpreting it. cheers
diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h index 51bfeb8777f0..af9b0e731f46 100644 --- a/arch/powerpc/include/asm/uaccess.h +++ b/arch/powerpc/include/asm/uaccess.h @@ -248,6 +248,7 @@ do { \ __chk_user_ptr(ptr); \ if (!is_kernel_addr((unsigned long)__gu_addr)) \ might_fault(); \ + barrier_nospec(); \ __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ (x) = (__typeof__(*(ptr)))__gu_val; \ __gu_err; \ @@ -258,8 +259,10 @@ do { \ long __gu_err = -EFAULT; \ unsigned long __gu_val = 0; \ const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ + int can_access = access_ok(VERIFY_READ, __gu_addr, (size)); \ might_fault(); \ - if (access_ok(VERIFY_READ, __gu_addr, (size))) \ + barrier_nospec(); \ + if (can_access) \ __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ (x) = (__force __typeof__(*(ptr)))__gu_val; \ __gu_err; \ @@ -271,6 +274,7 @@ do { \ unsigned long __gu_val; \ const __typeof__(*(ptr)) __user *__gu_addr = (ptr); \ __chk_user_ptr(ptr); \ + barrier_nospec(); \ __get_user_size(__gu_val, __gu_addr, (size), __gu_err); \ (x) = (__force __typeof__(*(ptr)))__gu_val; \ __gu_err; \ @@ -298,15 +302,19 @@ static inline unsigned long raw_copy_from_user(void *to, switch (n) { case 1: + barrier_nospec(); __get_user_size(*(u8 *)to, from, 1, ret); break; case 2: + barrier_nospec(); __get_user_size(*(u16 *)to, from, 2, ret); break; case 4: + barrier_nospec(); __get_user_size(*(u32 *)to, from, 4, ret); break; case 8: + barrier_nospec(); __get_user_size(*(u64 *)to, from, 8, ret); break; } @@ -314,6 +322,7 @@ static inline unsigned long raw_copy_from_user(void *to, return 0; } + barrier_nospec(); return __copy_tofrom_user((__force void __user *)to, from, n); }
This is based on x86 patch doing the same. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- arch/powerpc/include/asm/uaccess.h | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)