Re: [Xen-devel] [PATCH] x86: polish __{get,put}_user_{,no}check()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.05.17 at 16:28, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 02/05/17 14:23, Jan Beulich wrote:
>> The primary purpose is correcting a latent bug in __get_user_check()
>> (the macro has no active user at present): The access_ok() check should
>> be before the actual access, or else any PV guest could initiate MMIO
>> reads with side effects.
>>
>> Clean up all four macros at once:
>> - all arguments evaluated exactly once
>> - build the "check" flavor using the "nocheck" ones, instead of open
>>   coding them
>> - "int" is wide enough for error codes
>> - name local variables without using underscores as prefixes
>> - avoid pointless parentheses
>> - add blanks after commas separating parameters or arguments
>> - consistently use tabs for indentation
> 
> Could we use spaces?  This file is already half and half style, and
> these bits of code are a long way removed from their Linux heritage.

Well, if you're convinced this is better. I did consider doing so, but
didn't because it's a relatively small portion of code which does use
spaces at present.

>> --- a/xen/include/asm-x86/uaccess.h
>> +++ b/xen/include/asm-x86/uaccess.h
>> @@ -104,37 +104,35 @@ extern void __put_user_bad(void);
>>  #define __put_user(x,ptr) \
>>    __put_user_nocheck((__typeof__(*(ptr)))(x),(ptr),sizeof(*(ptr)))
>>  
>> -#define __put_user_nocheck(x,ptr,size)                              \
>> -({                                                          \
>> -    long __pu_err;                                          \
>> -    __put_user_size((x),(ptr),(size),__pu_err,-EFAULT);     \
>> -    __pu_err;                                               \
>> +#define __put_user_nocheck(x, ptr, size)                            \
>> +({                                                                  \
>> +    int err_;                                                       \
>> +    __put_user_size(x, ptr, size, err_, -EFAULT);                   \
>> +    err_;                                                           \
>>  })
>>  
>> -#define __put_user_check(x,ptr,size)                                        
>> \
>> +#define __put_user_check(x, ptr, size)                                      
>> \
>>  ({                                                                  \
>> -    long __pu_err = -EFAULT;                                        \
>> -    __typeof__(*(ptr)) __user *__pu_addr = (ptr);                   \
>> -    if (access_ok(__pu_addr,size))                                  \
>> -            __put_user_size((x),__pu_addr,(size),__pu_err,-EFAULT); \
>> -    __pu_err;                                                       \
>> +    __typeof__(*(ptr)) __user *ptr_ = (ptr);                        \
>> +    __typeof__(size) size_ = (size);                                \
>> +    access_ok(ptr_, size_) ? __put_user_nocheck(x, ptr_, size_)     \
>> +                           : -EFAULT;                               \
>>  })                                                  
> 
> Can you clobber the trailing whitespace on this line, like you did with
> __get_user_check() ?

Oh, sure. I didn't notice there was a similar issue here.

> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks, but please let me know whether you feel strongly about
using spaces instead of tabs.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel