From mboxrd@z Thu Jan  1 00:00:00 1970
From: =?ISO-8859-1?Q?Fernando_Luis_V=E1zquez_Cao?=
	<fernando@oss.ntt.co.jp>
Subject: Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty
 bitmaps
Date: Thu, 22 Apr 2010 11:45:54 +0900
Message-ID: <4BCFB862.7010509@oss.ntt.co.jp>
References: <20100420195349.dab60b1d.yoshikawa.takuya@oss.ntt.co.jp> <20100420200353.2d2a6dec.yoshikawa.takuya@oss.ntt.co.jp> <4BCEB777.6040505@oss.ntt.co.jp> <264937F8-98E4-4A73-9C7A-837793E1DAC8@suse.de>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: QUOTED-PRINTABLE
Cc: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>, avi@redhat.com,
	mtosatti@redhat.com, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org,
	kvm-ppc@vger.kernel.org
To: Alexander Graf <agraf@suse.de>
Return-path: <kvm-owner@vger.kernel.org>
Received: from serv2.oss.ntt.co.jp ([222.151.198.100]:46595 "EHLO
	serv2.oss.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754258Ab0DVCp4 (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 21 Apr 2010 22:45:56 -0400
In-Reply-To: <264937F8-98E4-4A73-9C7A-837793E1DAC8@suse.de>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 04/21/2010 06:41 PM, Alexander Graf wrote:
> On 21.04.2010, at 10:29, Fernando Luis V=E1zquez Cao wrote:
>=20
>> 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 __u=
64.
>=20
> So the high 32 bits are zero. Where's the problem?

If we are careful enough to cast the addr appropriately we should be fi=
ne,
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 =3D -EFAULT;
>>> +		if (copy_from_user(&log, argp, sizeof log))
>>> +			goto out;
>>> +		r =3D kvm_vm_ioctl_switch_dirty_log(kvm, &log);
>>> +		if (r)
>>> +			goto out;
>>> +		r =3D -EFAULT;
>>> +		if (copy_to_user(argp, &log, sizeof log))
>>> +			goto out;
>>> +		r =3D 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.
>=20
> 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.