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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/05/17 15:40, Jan Beulich wrote:
>>>> 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.

I'd prefer spaces (for overall consistency in the file), but my R-by
isn't conditional on it (as the file is already very mixed).

~Andrew

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