* Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl
@ 2020-02-28 1:36 linmiaohe
0 siblings, 0 replies; 10+ messages in thread
From: linmiaohe @ 2020-02-28 1:36 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm, linux-kernel, x86, pbonzini, rkrcmar, sean.j.christopherson,
wanpengli, jmattson, joro, tglx, mingo, bp, hpa
Vitaly Kuznetsov <vkuznets@redhat.com> writes:
>linmiaohe <linmiaohe@huawei.com> writes:
>
>> From: Miaohe Lin <linmiaohe@huawei.com>
>> - if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid)))
>> - goto out;
>> - r = 0;
>> + r = -EINVAL;
>> break;
>> }
>
>Braces are not really needed not but all other cases in the switch have it so let's leave them here too.
>
That's what I think too. :)
>>
>> case KVM_GET_MSRS: {
>> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
>
>"should not be used" pre-patch, post-patch we can say "Can only be used as a reliable source of -EINVAL" :-)
That's right.
>
> #define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
> /* Available with KVM_CAP_VAPIC */
> #define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct
> kvm_tpr_access_ctl)
>
>Surprisingly (or not), KVM_GET_CPUID2 is not even described in Documentation/virt/kvm/api.txt.
>
Maybe KVM_GET_CPUID2 is defined for integrity only.
>
>Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>
Many thanks for your review!
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl
2020-03-02 18:02 ` Paolo Bonzini
@ 2020-03-02 18:30 ` Jim Mattson
0 siblings, 0 replies; 10+ messages in thread
From: Jim Mattson @ 2020-03-02 18:30 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linmiaohe, Radim Krčmář,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Joerg Roedel,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin,
kvm list, LKML, the arch/x86 maintainers
On Mon, Mar 2, 2020 at 10:02 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 02/03/20 18:44, Jim Mattson wrote:
> > On Mon, Mar 2, 2020 at 9:09 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
> >>
> >> On 02/03/20 18:01, Jim Mattson wrote:
> >>>> And in fact, it's not used anywhere. So it should be
> >>>> deprecated.
> >>> I don't know how you can make the assertion that this ioctl is not
> >>> used anywhere. For instance, I see a use of it in Google's code base.
> >>
> >> Right, it does not seem to be used anywhere according to e.g. Debian
> >> code search but of course it can have users.
> >>
> >> What are you using it for? It's true that cpuid->nent is never written
> >> back to userspace, so the ioctl is basically unusable unless you already
> >> know how many entries are written. Or unless you fill the CPUID entries
> >> with garbage before calling it, I guess; is that what you are doing?
> >
> > One could use GET_CPUID2 after SET_CPUID2, to see what changes kvm
> > made to the requested guest CPUID information without telling you.
>
> Yeah, I think GET_CPUID2 with the same number of leaves that you have
> passed to SET_CPUID2 should work.
Having said that, it doesn't look like the method that invokes this
ioctl (in Google's code base) gets called from anywhere.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl
2020-03-02 17:44 ` Jim Mattson
@ 2020-03-02 18:02 ` Paolo Bonzini
2020-03-02 18:30 ` Jim Mattson
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2020-03-02 18:02 UTC (permalink / raw)
To: Jim Mattson
Cc: linmiaohe, Radim Krčmář,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Joerg Roedel,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin,
kvm list, LKML, the arch/x86 maintainers
On 02/03/20 18:44, Jim Mattson wrote:
> On Mon, Mar 2, 2020 at 9:09 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> On 02/03/20 18:01, Jim Mattson wrote:
>>>> And in fact, it's not used anywhere. So it should be
>>>> deprecated.
>>> I don't know how you can make the assertion that this ioctl is not
>>> used anywhere. For instance, I see a use of it in Google's code base.
>>
>> Right, it does not seem to be used anywhere according to e.g. Debian
>> code search but of course it can have users.
>>
>> What are you using it for? It's true that cpuid->nent is never written
>> back to userspace, so the ioctl is basically unusable unless you already
>> know how many entries are written. Or unless you fill the CPUID entries
>> with garbage before calling it, I guess; is that what you are doing?
>
> One could use GET_CPUID2 after SET_CPUID2, to see what changes kvm
> made to the requested guest CPUID information without telling you.
Yeah, I think GET_CPUID2 with the same number of leaves that you have
passed to SET_CPUID2 should work.
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl
2020-03-02 17:09 ` Paolo Bonzini
@ 2020-03-02 17:44 ` Jim Mattson
2020-03-02 18:02 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Jim Mattson @ 2020-03-02 17:44 UTC (permalink / raw)
To: Paolo Bonzini
Cc: linmiaohe, Radim Krčmář,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Joerg Roedel,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin,
kvm list, LKML, the arch/x86 maintainers
On Mon, Mar 2, 2020 at 9:09 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 02/03/20 18:01, Jim Mattson wrote:
> >> And in fact, it's not used anywhere. So it should be
> >> deprecated.
> > I don't know how you can make the assertion that this ioctl is not
> > used anywhere. For instance, I see a use of it in Google's code base.
>
> Right, it does not seem to be used anywhere according to e.g. Debian
> code search but of course it can have users.
>
> What are you using it for? It's true that cpuid->nent is never written
> back to userspace, so the ioctl is basically unusable unless you already
> know how many entries are written. Or unless you fill the CPUID entries
> with garbage before calling it, I guess; is that what you are doing?
One could use GET_CPUID2 after SET_CPUID2, to see what changes kvm
made to the requested guest CPUID information without telling you.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl
2020-03-02 17:01 ` Jim Mattson
@ 2020-03-02 17:09 ` Paolo Bonzini
2020-03-02 17:44 ` Jim Mattson
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2020-03-02 17:09 UTC (permalink / raw)
To: Jim Mattson, linmiaohe
Cc: Radim Krčmář,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Joerg Roedel,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin,
kvm list, LKML, the arch/x86 maintainers
On 02/03/20 18:01, Jim Mattson wrote:
>> And in fact, it's not used anywhere. So it should be
>> deprecated.
> I don't know how you can make the assertion that this ioctl is not
> used anywhere. For instance, I see a use of it in Google's code base.
Right, it does not seem to be used anywhere according to e.g. Debian
code search but of course it can have users.
What are you using it for? It's true that cpuid->nent is never written
back to userspace, so the ioctl is basically unusable unless you already
know how many entries are written. Or unless you fill the CPUID entries
with garbage before calling it, I guess; is that what you are doing?
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl
2020-02-27 3:21 linmiaohe
2020-02-27 11:36 ` Vitaly Kuznetsov
2020-03-02 16:31 ` Paolo Bonzini
@ 2020-03-02 17:01 ` Jim Mattson
2020-03-02 17:09 ` Paolo Bonzini
2 siblings, 1 reply; 10+ messages in thread
From: Jim Mattson @ 2020-03-02 17:01 UTC (permalink / raw)
To: linmiaohe
Cc: Paolo Bonzini, Radim Krčmář,
Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li, Joerg Roedel,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H . Peter Anvin,
kvm list, LKML, the arch/x86 maintainers
On Wed, Feb 26, 2020 at 7:20 PM linmiaohe <linmiaohe@huawei.com> wrote:
>
> From: Miaohe Lin <linmiaohe@huawei.com>
>
> When kvm_vcpu_ioctl_get_cpuid2() fails, we set cpuid->nent to the value of
> vcpu->arch.cpuid_nent. But this is in vain as cpuid->nent is not copied to
> userspace by copy_to_user() from call site. Also cpuid->nent is not updated
> to indicate how many entries were retrieved on success case. So this ioctl
> is straight up broken. And in fact, it's not used anywhere. So it should be
> deprecated.
I don't know how you can make the assertion that this ioctl is not
used anywhere. For instance, I see a use of it in Google's code base.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl
2020-02-27 3:21 linmiaohe
2020-02-27 11:36 ` Vitaly Kuznetsov
@ 2020-03-02 16:31 ` Paolo Bonzini
2020-03-02 17:01 ` Jim Mattson
2 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2020-03-02 16:31 UTC (permalink / raw)
To: linmiaohe, rkrcmar, sean.j.christopherson, vkuznets, wanpengli,
jmattson, joro, tglx, mingo, bp, hpa
Cc: kvm, linux-kernel, x86
On 27/02/20 04:21, linmiaohe wrote:
> From: Miaohe Lin <linmiaohe@huawei.com>
>
> When kvm_vcpu_ioctl_get_cpuid2() fails, we set cpuid->nent to the value of
> vcpu->arch.cpuid_nent. But this is in vain as cpuid->nent is not copied to
> userspace by copy_to_user() from call site. Also cpuid->nent is not updated
> to indicate how many entries were retrieved on success case. So this ioctl
> is straight up broken. And in fact, it's not used anywhere. So it should be
> deprecated.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
> arch/x86/kvm/cpuid.c | 20 --------------------
> arch/x86/kvm/cpuid.h | 3 ---
> arch/x86/kvm/x86.c | 16 ++--------------
> include/uapi/linux/kvm.h | 1 +
> tools/include/uapi/linux/kvm.h | 1 +
> 5 files changed, 4 insertions(+), 37 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..5e041a1282b8 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -261,26 +261,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> return r;
> }
>
> -int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> - struct kvm_cpuid2 *cpuid,
> - struct kvm_cpuid_entry2 __user *entries)
> -{
> - int r;
> -
> - r = -E2BIG;
> - if (cpuid->nent < vcpu->arch.cpuid_nent)
> - goto out;
> - r = -EFAULT;
> - if (copy_to_user(entries, &vcpu->arch.cpuid_entries,
> - vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
> - goto out;
> - return 0;
> -
> -out:
> - cpuid->nent = vcpu->arch.cpuid_nent;
> - return r;
> -}
> -
> static __always_inline void cpuid_mask(u32 *word, int wordnum)
> {
> reverse_cpuid_check(wordnum);
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 7366c618aa04..76555de38e1b 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -19,9 +19,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> struct kvm_cpuid2 *cpuid,
> struct kvm_cpuid_entry2 __user *entries);
> -int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> - struct kvm_cpuid2 *cpuid,
> - struct kvm_cpuid_entry2 __user *entries);
> bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
> u32 *ecx, u32 *edx, bool check_limit);
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ddd1d296bd20..a6d99abedb2c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4295,21 +4295,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> cpuid_arg->entries);
> break;
> }
> + /* KVM_GET_CPUID2 is deprecated, should not be used. */
> case KVM_GET_CPUID2: {
> - struct kvm_cpuid2 __user *cpuid_arg = argp;
> - struct kvm_cpuid2 cpuid;
> -
> - r = -EFAULT;
> - if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
> - goto out;
> - r = kvm_vcpu_ioctl_get_cpuid2(vcpu, &cpuid,
> - cpuid_arg->entries);
> - if (r)
> - goto out;
> - r = -EFAULT;
> - if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid)))
> - goto out;
> - r = 0;
> + r = -EINVAL;
> break;
> }
> case KVM_GET_MSRS: {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4b95f9a31a2f..61524780603d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1380,6 +1380,7 @@ struct kvm_s390_ucas_mapping {
> #define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
> #define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
> #define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
> #define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
> /* Available with KVM_CAP_VAPIC */
> #define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
> diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
> index f0a16b4adbbd..2ef719af4c57 100644
> --- a/tools/include/uapi/linux/kvm.h
> +++ b/tools/include/uapi/linux/kvm.h
> @@ -1379,6 +1379,7 @@ struct kvm_s390_ucas_mapping {
> #define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
> #define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
> #define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
> #define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
> /* Available with KVM_CAP_VAPIC */
> #define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
>
Queued, thanks.
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl
2020-02-27 11:36 ` Vitaly Kuznetsov
@ 2020-03-02 16:31 ` Paolo Bonzini
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2020-03-02 16:31 UTC (permalink / raw)
To: Vitaly Kuznetsov, linmiaohe, rkrcmar, sean.j.christopherson,
wanpengli, jmattson, joro, tglx, mingo, bp, hpa
Cc: kvm, linux-kernel, x86
On 27/02/20 12:36, Vitaly Kuznetsov wrote:
>> - if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid)))
>> - goto out;
>> - r = 0;
>> + r = -EINVAL;
>> break;
>> }
> Braces are not really needed not but all other cases in the switch have
> it so let's leave them here too.
>
We can remove the case altogether.
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl
2020-02-27 3:21 linmiaohe
@ 2020-02-27 11:36 ` Vitaly Kuznetsov
2020-03-02 16:31 ` Paolo Bonzini
2020-03-02 16:31 ` Paolo Bonzini
2020-03-02 17:01 ` Jim Mattson
2 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuznetsov @ 2020-02-27 11:36 UTC (permalink / raw)
To: linmiaohe, pbonzini, rkrcmar, sean.j.christopherson, wanpengli,
jmattson, joro, tglx, mingo, bp, hpa
Cc: linmiaohe, kvm, linux-kernel, x86
linmiaohe <linmiaohe@huawei.com> writes:
> From: Miaohe Lin <linmiaohe@huawei.com>
>
> When kvm_vcpu_ioctl_get_cpuid2() fails, we set cpuid->nent to the value of
> vcpu->arch.cpuid_nent. But this is in vain as cpuid->nent is not copied to
> userspace by copy_to_user() from call site. Also cpuid->nent is not updated
> to indicate how many entries were retrieved on success case. So this ioctl
> is straight up broken. And in fact, it's not used anywhere. So it should be
> deprecated.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
> arch/x86/kvm/cpuid.c | 20 --------------------
> arch/x86/kvm/cpuid.h | 3 ---
> arch/x86/kvm/x86.c | 16 ++--------------
> include/uapi/linux/kvm.h | 1 +
> tools/include/uapi/linux/kvm.h | 1 +
> 5 files changed, 4 insertions(+), 37 deletions(-)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index b1c469446b07..5e041a1282b8 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -261,26 +261,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> return r;
> }
>
> -int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> - struct kvm_cpuid2 *cpuid,
> - struct kvm_cpuid_entry2 __user *entries)
> -{
> - int r;
> -
> - r = -E2BIG;
> - if (cpuid->nent < vcpu->arch.cpuid_nent)
> - goto out;
> - r = -EFAULT;
> - if (copy_to_user(entries, &vcpu->arch.cpuid_entries,
> - vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
> - goto out;
> - return 0;
> -
> -out:
> - cpuid->nent = vcpu->arch.cpuid_nent;
> - return r;
> -}
> -
> static __always_inline void cpuid_mask(u32 *word, int wordnum)
> {
> reverse_cpuid_check(wordnum);
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 7366c618aa04..76555de38e1b 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -19,9 +19,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
> int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
> struct kvm_cpuid2 *cpuid,
> struct kvm_cpuid_entry2 __user *entries);
> -int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
> - struct kvm_cpuid2 *cpuid,
> - struct kvm_cpuid_entry2 __user *entries);
> bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
> u32 *ecx, u32 *edx, bool check_limit);
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ddd1d296bd20..a6d99abedb2c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -4295,21 +4295,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> cpuid_arg->entries);
> break;
> }
> + /* KVM_GET_CPUID2 is deprecated, should not be used. */
> case KVM_GET_CPUID2: {
> - struct kvm_cpuid2 __user *cpuid_arg = argp;
> - struct kvm_cpuid2 cpuid;
> -
> - r = -EFAULT;
> - if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
> - goto out;
> - r = kvm_vcpu_ioctl_get_cpuid2(vcpu, &cpuid,
> - cpuid_arg->entries);
> - if (r)
> - goto out;
> - r = -EFAULT;
> - if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid)))
> - goto out;
> - r = 0;
> + r = -EINVAL;
> break;
> }
Braces are not really needed not but all other cases in the switch have
it so let's leave them here too.
> case KVM_GET_MSRS: {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 4b95f9a31a2f..61524780603d 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1380,6 +1380,7 @@ struct kvm_s390_ucas_mapping {
> #define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
> #define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
> #define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
> #define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
> /* Available with KVM_CAP_VAPIC */
> #define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
> diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
> index f0a16b4adbbd..2ef719af4c57 100644
> --- a/tools/include/uapi/linux/kvm.h
> +++ b/tools/include/uapi/linux/kvm.h
> @@ -1379,6 +1379,7 @@ struct kvm_s390_ucas_mapping {
> #define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
> #define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
> #define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
> +/* KVM_GET_CPUID2 is deprecated, should not be used. */
"should not be used" pre-patch, post-patch we can say "Can only be used
as a reliable source of -EINVAL" :-)
> #define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
> /* Available with KVM_CAP_VAPIC */
> #define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
Surprisingly (or not), KVM_GET_CPUID2 is not even described in
Documentation/virt/kvm/api.txt.
Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
--
Vitaly
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl
@ 2020-02-27 3:21 linmiaohe
2020-02-27 11:36 ` Vitaly Kuznetsov
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: linmiaohe @ 2020-02-27 3:21 UTC (permalink / raw)
To: pbonzini, rkrcmar, sean.j.christopherson, vkuznets, wanpengli,
jmattson, joro, tglx, mingo, bp, hpa
Cc: linmiaohe, kvm, linux-kernel, x86
From: Miaohe Lin <linmiaohe@huawei.com>
When kvm_vcpu_ioctl_get_cpuid2() fails, we set cpuid->nent to the value of
vcpu->arch.cpuid_nent. But this is in vain as cpuid->nent is not copied to
userspace by copy_to_user() from call site. Also cpuid->nent is not updated
to indicate how many entries were retrieved on success case. So this ioctl
is straight up broken. And in fact, it's not used anywhere. So it should be
deprecated.
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
arch/x86/kvm/cpuid.c | 20 --------------------
arch/x86/kvm/cpuid.h | 3 ---
arch/x86/kvm/x86.c | 16 ++--------------
include/uapi/linux/kvm.h | 1 +
tools/include/uapi/linux/kvm.h | 1 +
5 files changed, 4 insertions(+), 37 deletions(-)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index b1c469446b07..5e041a1282b8 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -261,26 +261,6 @@ int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
return r;
}
-int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
- struct kvm_cpuid2 *cpuid,
- struct kvm_cpuid_entry2 __user *entries)
-{
- int r;
-
- r = -E2BIG;
- if (cpuid->nent < vcpu->arch.cpuid_nent)
- goto out;
- r = -EFAULT;
- if (copy_to_user(entries, &vcpu->arch.cpuid_entries,
- vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
- goto out;
- return 0;
-
-out:
- cpuid->nent = vcpu->arch.cpuid_nent;
- return r;
-}
-
static __always_inline void cpuid_mask(u32 *word, int wordnum)
{
reverse_cpuid_check(wordnum);
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 7366c618aa04..76555de38e1b 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -19,9 +19,6 @@ int kvm_vcpu_ioctl_set_cpuid(struct kvm_vcpu *vcpu,
int kvm_vcpu_ioctl_set_cpuid2(struct kvm_vcpu *vcpu,
struct kvm_cpuid2 *cpuid,
struct kvm_cpuid_entry2 __user *entries);
-int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
- struct kvm_cpuid2 *cpuid,
- struct kvm_cpuid_entry2 __user *entries);
bool kvm_cpuid(struct kvm_vcpu *vcpu, u32 *eax, u32 *ebx,
u32 *ecx, u32 *edx, bool check_limit);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index ddd1d296bd20..a6d99abedb2c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4295,21 +4295,9 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
cpuid_arg->entries);
break;
}
+ /* KVM_GET_CPUID2 is deprecated, should not be used. */
case KVM_GET_CPUID2: {
- struct kvm_cpuid2 __user *cpuid_arg = argp;
- struct kvm_cpuid2 cpuid;
-
- r = -EFAULT;
- if (copy_from_user(&cpuid, cpuid_arg, sizeof(cpuid)))
- goto out;
- r = kvm_vcpu_ioctl_get_cpuid2(vcpu, &cpuid,
- cpuid_arg->entries);
- if (r)
- goto out;
- r = -EFAULT;
- if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid)))
- goto out;
- r = 0;
+ r = -EINVAL;
break;
}
case KVM_GET_MSRS: {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 4b95f9a31a2f..61524780603d 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1380,6 +1380,7 @@ struct kvm_s390_ucas_mapping {
#define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
#define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
#define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
+/* KVM_GET_CPUID2 is deprecated, should not be used. */
#define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
/* Available with KVM_CAP_VAPIC */
#define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
diff --git a/tools/include/uapi/linux/kvm.h b/tools/include/uapi/linux/kvm.h
index f0a16b4adbbd..2ef719af4c57 100644
--- a/tools/include/uapi/linux/kvm.h
+++ b/tools/include/uapi/linux/kvm.h
@@ -1379,6 +1379,7 @@ struct kvm_s390_ucas_mapping {
#define KVM_GET_LAPIC _IOR(KVMIO, 0x8e, struct kvm_lapic_state)
#define KVM_SET_LAPIC _IOW(KVMIO, 0x8f, struct kvm_lapic_state)
#define KVM_SET_CPUID2 _IOW(KVMIO, 0x90, struct kvm_cpuid2)
+/* KVM_GET_CPUID2 is deprecated, should not be used. */
#define KVM_GET_CPUID2 _IOWR(KVMIO, 0x91, struct kvm_cpuid2)
/* Available with KVM_CAP_VAPIC */
#define KVM_TPR_ACCESS_REPORTING _IOWR(KVMIO, 0x92, struct kvm_tpr_access_ctl)
--
2.19.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-03-02 18:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-28 1:36 [PATCH v2] KVM: X86: deprecate obsolete KVM_GET_CPUID2 ioctl linmiaohe
-- strict thread matches above, loose matches on Subject: below --
2020-02-27 3:21 linmiaohe
2020-02-27 11:36 ` Vitaly Kuznetsov
2020-03-02 16:31 ` Paolo Bonzini
2020-03-02 16:31 ` Paolo Bonzini
2020-03-02 17:01 ` Jim Mattson
2020-03-02 17:09 ` Paolo Bonzini
2020-03-02 17:44 ` Jim Mattson
2020-03-02 18:02 ` Paolo Bonzini
2020-03-02 18:30 ` Jim Mattson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).