Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty

On 04/21/2010 06:41 PM, Alexander Graf wrote:
> On 21.04.2010, at 10:29, Fernando Luis Vázquez Cao wrote:
> 
>> On 04/20/2010 08:03 PM, Takuya Yoshikawa wrote:
>>> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
>>> 	__u32 padding1;
>>> 	union {
>>> 		void __user *dirty_bitmap; /* one bit per page */
>>> -		__u64 padding2;
>>> +		__u64 addr;
>>
>> This can break on x86_32 and x86_64-compat. addr is a long not a __u64.
> 
> So the high 32 bits are zero. Where's the problem?

If we are careful enough to cast the addr appropriately we should be fine,
even if we keep the padding field in the union. I am not saying that it
breaks 32 architectures but that it can potentially be problematic.

>>> +	case KVM_SWITCH_DIRTY_LOG: {
>>> +		struct kvm_dirty_log log;
>>> +
>>> +		r = -EFAULT;
>>> +		if (copy_from_user(&log, argp, sizeof log))
>>> +			goto out;
>>> +		r = kvm_vm_ioctl_switch_dirty_log(kvm, &log);
>>> +		if (r)
>>> +			goto out;
>>> +		r = -EFAULT;
>>> +		if (copy_to_user(argp, &log, sizeof log))
>>> +			goto out;
>>> +		r = 0;
>>> +		break;
>>> +	}
>>
>> In x86_64-compat mode we are handling 32bit user-space addresses
>> so we need the compat counterpart of KVM_SWITCH_DIRTY_LOG too.
> 
> The compat code just forwards everything to the generic ioctls.

The compat code uses struct compat_kvm_dirty_log instead of
struct kvm_dirty_log to communicate with user space so
the necessary conversions needs to be done before invoking
the generic ioctl (see KVM_GET_DIRTY_LOG in kvm_vm_compat_ioctl).

By the way we probable should move the definition of struct
compat_kvm_dirty_log to a header file.