Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 26 May 2023 13:25:18 -0700
In-Reply-To: <20230413175844.21760-1-itazur@amazon.com>
Mime-Version: 1.0
References: <20230413175844.21760-1-itazur@amazon.com>
Message-ID: <ZHEVrowZsi2j6dVI@google.com>
Subject: Re: [PATCH v2] KVM: x86: Update KVM_GET_CPUID2 to return valid entry count
From:   Sean Christopherson <seanjc@google.com>
To:     Takahiro Itazuri <itazur@amazon.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        linux-kernel@vger.kernel.org, Takahiro Itazuri <zulinx86@gmail.com>
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

On Thu, Apr 13, 2023, Takahiro Itazuri wrote:
> Modify the KVM_GET_CPUID2 API to return the number of valid entries in
> nent field of kvm_cpuid2 even on success.
> 
> Previously, the KVM_GET_CPUID2 API only updated the nent field when an
> error was returned. If the API was called with an entry count larger
> than necessary (e.g., KVM_MAX_CPUID_ENTRIES), it would succeed, but the
> nent field would continue to show a value larger than the actual number
> of entries filled by the KVM_GET_CPUID2 API. With this change, users can
> rely on the updated nent field and there is no need to traverse
> unnecessary entries and check whether an entry is valid or not.
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Takahiro Itazuri <itazur@amazon.com>
> ---
> Changes
> v1 -> v2
> * Capitalize "kvm" in the commit title.
> * Remove a scratch "nent" variable.
> * Link to v1: https://lore.kernel.org/all/20230410141820.57328-1-itazur@amazon.com/
> 
>  arch/x86/kvm/cpuid.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 599aebec2d52..20d28ebdc672 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -523,18 +523,18 @@ int kvm_vcpu_ioctl_get_cpuid2(struct kvm_vcpu *vcpu,
>  			      struct kvm_cpuid2 *cpuid,
>  			      struct kvm_cpuid_entry2 __user *entries)
>  {
> -	int r;
> +	int r = 0;
>  
> -	r = -E2BIG;
>  	if (cpuid->nent < vcpu->arch.cpuid_nent)
> -		goto out;
> -	r = -EFAULT;
> -	if (copy_to_user(entries, vcpu->arch.cpuid_entries,
> +		r = -E2BIG;
> +	else if (copy_to_user(entries, vcpu->arch.cpuid_entries,
>  			 vcpu->arch.cpuid_nent * sizeof(struct kvm_cpuid_entry2)))
> -		goto out;
> -	return 0;
> +		r = -EFAULT;
>  
> -out:
> +	/*
> +	 * Update "nent" even on failure, e.g. so that userspace can fix an
> +	 * -E2BIG issue by allocating a larger array.
> +	 */

Gah, this is wrong.  The caller only copies @cpuid to userspace on success.  My
fault for not seeing this in v1, but this is exactly why we write tests.

I've written a test and a fix, I'll post 'em shortly.