From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Fernando_Luis_V=E1zquez_Cao?= 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 , avi@redhat.com, mtosatti@redhat.com, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org, kvm-ppc@vger.kernel.org To: Alexander Graf Return-path: 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 ); 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: 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Fernando_Luis_V=E1zquez_Cao?= Date: Thu, 22 Apr 2010 02:45:54 +0000 Subject: Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Message-Id: <4BCFB862.7010509@oss.ntt.co.jp> List-Id: 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> In-Reply-To: <264937F8-98E4-4A73-9C7A-837793E1DAC8@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Alexander Graf Cc: Takuya Yoshikawa , avi@redhat.com, mtosatti@redhat.com, kvm@vger.kernel.org, kvm-ia64@vger.kernel.org, kvm-ppc@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 __u64. >=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 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 =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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Fernando_Luis_V=E1zquez_Cao?= Date: Thu, 22 Apr 2010 02:45:54 +0000 Subject: Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty Message-Id: <4BCFB862.7010509@oss.ntt.co.jp> List-Id: References: <20100420200353.2d2a6dec.yoshikawa.takuya@oss.ntt.co.jp> In-Reply-To: <20100420200353.2d2a6dec.yoshikawa.takuya@oss.ntt.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: kvm-ia64@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 __u64. >=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 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 =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.