| Message ID | 20230203094230.266952-7-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Standardize on "int" return types instead of "long" | expand |
Hi Thomas, On 2/3/23 8:42 PM, Thomas Huth wrote: > This function only returns normal integer values, so there is > no need to declare its return value as "long". > > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > arch/arm64/include/asm/kvm_host.h | 4 ++-- > arch/arm64/kvm/guest.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 35a159d131b5..b1a16343767f 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -963,8 +963,8 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > struct kvm_device_attr *attr); > > -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > - struct kvm_arm_copy_mte_tags *copy_tags); > +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > + struct kvm_arm_copy_mte_tags *copy_tags); > > /* Guest/host FPSIMD coordination helpers */ > int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index cf4c495a4321..80e530549c34 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -1013,8 +1013,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > return ret; > } > > -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > - struct kvm_arm_copy_mte_tags *copy_tags) > +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > + struct kvm_arm_copy_mte_tags *copy_tags) > { > gpa_t guest_ipa = copy_tags->guest_ipa; > size_t length = copy_tags->length; > It's possible for the function to return number of bytes have been copied. Its type is 'size_t', same to 'unsigned long'. So 'int' doesn't have sufficient space for it if I'm correct. long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, struct kvm_arm_copy_mte_tags *copy_tags) { gpa_t guest_ipa = copy_tags->guest_ipa; size_t length = copy_tags->length; : : out: mutex_unlock(&kvm->slots_lock); /* If some data has been copied report the number of bytes copied */ if (length != copy_tags->length) return copy_tags->length - length; return ret; } Thanks, Gavin
On 07/02/2023 01.09, Gavin Shan wrote: > Hi Thomas, > > On 2/3/23 8:42 PM, Thomas Huth wrote: >> This function only returns normal integer values, so there is >> no need to declare its return value as "long". >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> arch/arm64/include/asm/kvm_host.h | 4 ++-- >> arch/arm64/kvm/guest.c | 4 ++-- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm64/include/asm/kvm_host.h >> b/arch/arm64/include/asm/kvm_host.h >> index 35a159d131b5..b1a16343767f 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -963,8 +963,8 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, >> int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, >> struct kvm_device_attr *attr); >> -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >> - struct kvm_arm_copy_mte_tags *copy_tags); >> +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >> + struct kvm_arm_copy_mte_tags *copy_tags); >> /* Guest/host FPSIMD coordination helpers */ >> int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); >> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c >> index cf4c495a4321..80e530549c34 100644 >> --- a/arch/arm64/kvm/guest.c >> +++ b/arch/arm64/kvm/guest.c >> @@ -1013,8 +1013,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, >> return ret; >> } >> -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >> - struct kvm_arm_copy_mte_tags *copy_tags) >> +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >> + struct kvm_arm_copy_mte_tags *copy_tags) >> { >> gpa_t guest_ipa = copy_tags->guest_ipa; >> size_t length = copy_tags->length; >> > > It's possible for the function to return number of bytes have been copied. > Its type is 'size_t', same to 'unsigned long'. So 'int' doesn't have sufficient > space for it if I'm correct. > > long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > struct kvm_arm_copy_mte_tags *copy_tags) > { > gpa_t guest_ipa = copy_tags->guest_ipa; > size_t length = copy_tags->length; > : > : > out: > mutex_unlock(&kvm->slots_lock); > /* If some data has been copied report the number of bytes copied */ > if (length != copy_tags->length) > return copy_tags->length - length; > return ret; > } Oh, drat, I thought I had checked all return statements ... this must have fallen through the cracks, sorry! Anyway, this is already a problem now: The function is called from kvm_arch_vm_ioctl() (which still returns a long), which in turn is called from kvm_vm_ioctl() in virt/kvm/kvm_main.c. And that functions stores the return value in an "int r" variable. So the upper bits are already lost there. Also, how is this supposed to work from user space? The normal "ioctl()" libc function just returns an "int" ? Is this ioctl already used in a userspace application somewhere? ... at least in QEMU, I didn't spot it yet... Thomas
On 2/7/23 9:09 PM, Thomas Huth wrote: > On 07/02/2023 01.09, Gavin Shan wrote: >> Hi Thomas, >> >> On 2/3/23 8:42 PM, Thomas Huth wrote: >>> This function only returns normal integer values, so there is >>> no need to declare its return value as "long". >>> >>> Signed-off-by: Thomas Huth <thuth@redhat.com> >>> --- >>> arch/arm64/include/asm/kvm_host.h | 4 ++-- >>> arch/arm64/kvm/guest.c | 4 ++-- >>> 2 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>> index 35a159d131b5..b1a16343767f 100644 >>> --- a/arch/arm64/include/asm/kvm_host.h >>> +++ b/arch/arm64/include/asm/kvm_host.h >>> @@ -963,8 +963,8 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, >>> int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, >>> struct kvm_device_attr *attr); >>> -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >>> - struct kvm_arm_copy_mte_tags *copy_tags); >>> +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >>> + struct kvm_arm_copy_mte_tags *copy_tags); >>> /* Guest/host FPSIMD coordination helpers */ >>> int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); >>> diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c >>> index cf4c495a4321..80e530549c34 100644 >>> --- a/arch/arm64/kvm/guest.c >>> +++ b/arch/arm64/kvm/guest.c >>> @@ -1013,8 +1013,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, >>> return ret; >>> } >>> -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >>> - struct kvm_arm_copy_mte_tags *copy_tags) >>> +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >>> + struct kvm_arm_copy_mte_tags *copy_tags) >>> { >>> gpa_t guest_ipa = copy_tags->guest_ipa; >>> size_t length = copy_tags->length; >>> >> >> It's possible for the function to return number of bytes have been copied. >> Its type is 'size_t', same to 'unsigned long'. So 'int' doesn't have sufficient >> space for it if I'm correct. >> >> long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, >> struct kvm_arm_copy_mte_tags *copy_tags) >> { >> gpa_t guest_ipa = copy_tags->guest_ipa; >> size_t length = copy_tags->length; >> : >> : >> out: >> mutex_unlock(&kvm->slots_lock); >> /* If some data has been copied report the number of bytes copied */ >> if (length != copy_tags->length) >> return copy_tags->length - length; >> return ret; >> } > > Oh, drat, I thought I had checked all return statements ... this must have fallen through the cracks, sorry! > > Anyway, this is already a problem now: The function is called from kvm_arch_vm_ioctl() (which still returns a long), which in turn is called from kvm_vm_ioctl() in virt/kvm/kvm_main.c. And that functions stores the return value in an "int r" variable. So the upper bits are already lost there. > > Also, how is this supposed to work from user space? The normal "ioctl()" libc function just returns an "int" ? Is this ioctl already used in a userspace application somewhere? ... at least in QEMU, I didn't spot it yet... > The ioctl command KVM_ARM_MTE_COPY_TAGS was merged recently and not used by QEMU yet. I think struct kvm_arm_copy_mte_tags::length needs to be '__u32' instead of '__u64' in order to standardize the return value. Something like below. Documentation/virt/kvm/api.rst::section-4.130 needs update accordingly. struct kvm_arm_copy_mte_tags { __u64 guest_ipa; __u32 pad; __u32 length; void __user *addr; __u64 flags; __u64 reserved[2]; }; Thanks, Gavin
On Wed, Feb 08 2023, Gavin Shan <gshan@redhat.com> wrote: > On 2/7/23 9:09 PM, Thomas Huth wrote: >> Oh, drat, I thought I had checked all return statements ... this must have fallen through the cracks, sorry! >> >> Anyway, this is already a problem now: The function is called from kvm_arch_vm_ioctl() (which still returns a long), which in turn is called from kvm_vm_ioctl() in virt/kvm/kvm_main.c. And that functions stores the return value in an "int r" variable. So the upper bits are already lost there. >> >> Also, how is this supposed to work from user space? The normal "ioctl()" libc function just returns an "int" ? Is this ioctl already used in a userspace application somewhere? ... at least in QEMU, I didn't spot it yet... >> We will need it in QEMU to implement migration with MTE (the current proposal simply adds a migration blocker when MTE is enabled, as there are various other things that need to be figured out for this to work.) But maybe other VMMs already use it (and have been lucky because they always dealt with shorter lengths?) > > The ioctl command KVM_ARM_MTE_COPY_TAGS was merged recently and not used > by QEMU yet. I think struct kvm_arm_copy_mte_tags::length needs to be > '__u32' instead of '__u64' in order to standardize the return value. > Something like below. Documentation/virt/kvm/api.rst::section-4.130 > needs update accordingly. > > struct kvm_arm_copy_mte_tags { > __u64 guest_ipa; > __u32 pad; > __u32 length; > void __user *addr; > __u64 flags; > __u64 reserved[2]; > }; Can we do this in a more compatible way, as we are dealing with an API? Like returning -EINVAL if length is too big?
On 08/02/2023 08:49, Cornelia Huck wrote: > On Wed, Feb 08 2023, Gavin Shan <gshan@redhat.com> wrote: > >> On 2/7/23 9:09 PM, Thomas Huth wrote: >>> Oh, drat, I thought I had checked all return statements ... this must have fallen through the cracks, sorry! >>> >>> Anyway, this is already a problem now: The function is called from kvm_arch_vm_ioctl() (which still returns a long), which in turn is called from kvm_vm_ioctl() in virt/kvm/kvm_main.c. And that functions stores the return value in an "int r" variable. So the upper bits are already lost there. Sorry about that, I was caught out by kvm_arch_vm_ioctl() returning long... >>> Also, how is this supposed to work from user space? The normal "ioctl()" libc function just returns an "int" ? Is this ioctl already used in a userspace application somewhere? ... at least in QEMU, I didn't spot it yet... >>> > > We will need it in QEMU to implement migration with MTE (the current > proposal simply adds a migration blocker when MTE is enabled, as there > are various other things that need to be figured out for this to work.) > But maybe other VMMs already use it (and have been lucky because they > always dealt with shorter lengths?) > >> >> The ioctl command KVM_ARM_MTE_COPY_TAGS was merged recently and not used >> by QEMU yet. I think struct kvm_arm_copy_mte_tags::length needs to be >> '__u32' instead of '__u64' in order to standardize the return value. >> Something like below. Documentation/virt/kvm/api.rst::section-4.130 >> needs update accordingly. >> >> struct kvm_arm_copy_mte_tags { >> __u64 guest_ipa; >> __u32 pad; >> __u32 length; >> void __user *addr; >> __u64 flags; >> __u64 reserved[2]; >> }; > > Can we do this in a more compatible way, as we are dealing with an API? > Like returning -EINVAL if length is too big? > I agree the simplest fix for the problem is simply to reject any lengths>INT_MAX: diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index cf4c495a4321..94aed7ce85c4 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -1032,6 +1032,13 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, if (copy_tags->flags & ~KVM_ARM_TAGS_FROM_GUEST) return -EINVAL; + /* + * ioctl returns int, so lengths above INT_MAX cannot be + * represented in the return value + */ + if (length > INT_MAX) + return -EINVAL; + if (length & ~PAGE_MASK || guest_ipa & ~PAGE_MASK) return -EINVAL; This could also be fixed in a useable way by including a new flag which returns the length in an output field of the ioctl structure. I'm guessing a 2GB limit would be annoying to work around. Steve
On 08/02/2023 12.51, Steven Price wrote: > On 08/02/2023 08:49, Cornelia Huck wrote: >> On Wed, Feb 08 2023, Gavin Shan <gshan@redhat.com> wrote: >> >>> On 2/7/23 9:09 PM, Thomas Huth wrote: >>>> Oh, drat, I thought I had checked all return statements ... this must have fallen through the cracks, sorry! >>>> >>>> Anyway, this is already a problem now: The function is called from kvm_arch_vm_ioctl() (which still returns a long), which in turn is called from kvm_vm_ioctl() in virt/kvm/kvm_main.c. And that functions stores the return value in an "int r" variable. So the upper bits are already lost there. > > Sorry about that, I was caught out by kvm_arch_vm_ioctl() returning long... That's why I'm trying to fix that return type mess with my series, to avoid such problems in the future :-) >>>> Also, how is this supposed to work from user space? The normal "ioctl()" libc function just returns an "int" ? Is this ioctl already used in a userspace application somewhere? ... at least in QEMU, I didn't spot it yet... >>>> >> >> We will need it in QEMU to implement migration with MTE (the current >> proposal simply adds a migration blocker when MTE is enabled, as there >> are various other things that need to be figured out for this to work.) >> But maybe other VMMs already use it (and have been lucky because they >> always dealt with shorter lengths?) >> >>> >>> The ioctl command KVM_ARM_MTE_COPY_TAGS was merged recently and not used >>> by QEMU yet. I think struct kvm_arm_copy_mte_tags::length needs to be >>> '__u32' instead of '__u64' in order to standardize the return value. >>> Something like below. Documentation/virt/kvm/api.rst::section-4.130 >>> needs update accordingly. >>> >>> struct kvm_arm_copy_mte_tags { >>> __u64 guest_ipa; >>> __u32 pad; >>> __u32 length; >>> void __user *addr; >>> __u64 flags; >>> __u64 reserved[2]; >>> }; >> >> Can we do this in a more compatible way, as we are dealing with an API? >> Like returning -EINVAL if length is too big? >> > > I agree the simplest fix for the problem is simply to reject any > lengths>INT_MAX: > > diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c > index cf4c495a4321..94aed7ce85c4 100644 > --- a/arch/arm64/kvm/guest.c > +++ b/arch/arm64/kvm/guest.c > @@ -1032,6 +1032,13 @@ long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, > if (copy_tags->flags & ~KVM_ARM_TAGS_FROM_GUEST) > return -EINVAL; > > + /* > + * ioctl returns int, so lengths above INT_MAX cannot be > + * represented in the return value > + */ > + if (length > INT_MAX) > + return -EINVAL; > + > if (length & ~PAGE_MASK || guest_ipa & ~PAGE_MASK) > return -EINVAL; > > This could also be fixed in a useable way by including a new flag which > returns the length in an output field of the ioctl structure. I'm > guessing a 2GB limit would be annoying to work around. I agree that checking for length > INT_MAX is likely the best thing to do here right now. I'll add that in v2 of my series. But actually, this might even be a good thing from another point of view (so I'm not sure whether your idea with the flag should really be pursued): The code here takes a mutex and then runs a while loop that depends on the length - which could cause the lock to be held for a rather long time if length is a 64-bit value. Forcing the user space to limit the length here could help to avoid taking the lock for too long. Thomas
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 35a159d131b5..b1a16343767f 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -963,8 +963,8 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr); -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, - struct kvm_arm_copy_mte_tags *copy_tags); +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, + struct kvm_arm_copy_mte_tags *copy_tags); /* Guest/host FPSIMD coordination helpers */ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu); diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c index cf4c495a4321..80e530549c34 100644 --- a/arch/arm64/kvm/guest.c +++ b/arch/arm64/kvm/guest.c @@ -1013,8 +1013,8 @@ int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, return ret; } -long kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, - struct kvm_arm_copy_mte_tags *copy_tags) +int kvm_vm_ioctl_mte_copy_tags(struct kvm *kvm, + struct kvm_arm_copy_mte_tags *copy_tags) { gpa_t guest_ipa = copy_tags->guest_ipa; size_t length = copy_tags->length;
This function only returns normal integer values, so there is no need to declare its return value as "long". Signed-off-by: Thomas Huth <thuth@redhat.com> --- arch/arm64/include/asm/kvm_host.h | 4 ++-- arch/arm64/kvm/guest.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)