On 22.04.2010, at 08:09, Fernando Luis Vázquez Cao wrote:
> On 04/22/2010 11:45 AM, Fernando Luis Vázquez Cao wrote:
>> 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.
>
> It seems that it was you and Arnd who added the kvm_vm compat ioctl :-).
> Are you considering a different approach to tackle the issues that we
> have with a big-endian userspace?
IIRC the issue was a pointer inside of a nested structure, no?
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
