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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 02/05/17 15:40, Jan Beulich wrote:
>>>> On 02.05.17 at 16:28, <andrew.cooper3@citrix.com> 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@citrix.com>
> 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@lists.xen.org
https://lists.xen.org/xen-devel