Re: [PATCH RFC rebase 2/9] powerpc: Use barrier_nospec in copy_from_user

[Michael Ellerman]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:

> On Thu, Mar 15, 2018 at 12:15 PM, Michal Suchanek <msuchanek@xxxxxxx> wrote:
>> This is based on x86 patch doing the same.
>>
>> Signed-off-by: Michal Suchanek <msuchanek@xxxxxxx>
>> ---
>> --- 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