diff mbox series

[2/7] KVM: x86: Improve return type handling in kvm_vm_ioctl_get_nr_mmu_pages()

Message ID 20230203094230.266952-3-thuth@redhat.com
State New
Headers
Series KVM: Standardize on "int" return types instead of "long" |

Commit Message

Thomas Huth Feb. 3, 2023, 9:42 a.m. UTC 
  kvm_vm_ioctl_get_nr_mmu_pages() tries to return a "unsigned long" value,
but its caller only stores ther return value in an "int" - which is also
what all the other kvm_vm_ioctl_*() functions are returning. So returning
values that do not fit into a 32-bit integer anymore does not work here.
It's better to adjust the return type, add a sanity check and return an
error instead if the value is too big.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 arch/x86/kvm/x86.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Sean Christopherson Feb. 3, 2023, 5:48 p.m. UTC | #1 
On Fri, Feb 03, 2023, Thomas Huth wrote:
> kvm_vm_ioctl_get_nr_mmu_pages() tries to return a "unsigned long" value,
> but its caller only stores ther return value in an "int" - which is also
> what all the other kvm_vm_ioctl_*() functions are returning. So returning
> values that do not fit into a 32-bit integer anymore does not work here.
> It's better to adjust the return type, add a sanity check and return an
> error instead if the value is too big.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  arch/x86/kvm/x86.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index da4bbd043a7b..caa2541833dd 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -6007,8 +6007,11 @@ static int kvm_vm_ioctl_set_nr_mmu_pages(struct kvm *kvm,
>  	return 0;
>  }
>  
> -static unsigned long kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
> +static int kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
>  {
> +	if (kvm->arch.n_max_mmu_pages > INT_MAX)
> +		return -EOVERFLOW;
> +
>  	return kvm->arch.n_max_mmu_pages;
>  }

My vote is to skip this patch, skip deprecation, and go straight to deleting
KVM_GET_NR_MMU_PAGES.  The ioctl() has never worked[*], and none of the VMMs I
checked use it (QEMU, Google's internal VMM, kvmtool, CrosVM).

[*] https://lore.kernel.org/all/YpZu6%2Fk+8EydfBKf@google.com
Thomas Huth Feb. 7, 2023, 9:26 a.m. UTC | #2 
On 03/02/2023 18.48, Sean Christopherson wrote:
> On Fri, Feb 03, 2023, Thomas Huth wrote:
>> kvm_vm_ioctl_get_nr_mmu_pages() tries to return a "unsigned long" value,
>> but its caller only stores ther return value in an "int" - which is also
>> what all the other kvm_vm_ioctl_*() functions are returning. So returning
>> values that do not fit into a 32-bit integer anymore does not work here.
>> It's better to adjust the return type, add a sanity check and return an
>> error instead if the value is too big.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   arch/x86/kvm/x86.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index da4bbd043a7b..caa2541833dd 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -6007,8 +6007,11 @@ static int kvm_vm_ioctl_set_nr_mmu_pages(struct kvm *kvm,
>>   	return 0;
>>   }
>>   
>> -static unsigned long kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
>> +static int kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
>>   {
>> +	if (kvm->arch.n_max_mmu_pages > INT_MAX)
>> +		return -EOVERFLOW;
>> +
>>   	return kvm->arch.n_max_mmu_pages;
>>   }
> 
> My vote is to skip this patch, skip deprecation, and go straight to deleting
> KVM_GET_NR_MMU_PAGES.  The ioctl() has never worked[*], and none of the VMMs I
> checked use it (QEMU, Google's internal VMM, kvmtool, CrosVM).

I guess I'm living too much in the QEMU world where things need to be 
deprecated first before removing them ;-)
But sure, if everybody agrees that removing this directly is fine, too, I 
can do this in v2.

  Thomas


PS: Has there ever been a discussion about the other deprecated interfaces 
in include/uapi/linux/kvm.h ? Most of the stuff there seems to be from 2009 
... so maybe it's time now to remove that, too?
Sean Christopherson Feb. 7, 2023, 4:25 p.m. UTC | #3 
On Tue, Feb 07, 2023, Thomas Huth wrote:
> On 03/02/2023 18.48, Sean Christopherson wrote:
> > On Fri, Feb 03, 2023, Thomas Huth wrote:
> > > kvm_vm_ioctl_get_nr_mmu_pages() tries to return a "unsigned long" value,
> > > but its caller only stores ther return value in an "int" - which is also
> > > what all the other kvm_vm_ioctl_*() functions are returning. So returning
> > > values that do not fit into a 32-bit integer anymore does not work here.
> > > It's better to adjust the return type, add a sanity check and return an
> > > error instead if the value is too big.
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > ---
> > >   arch/x86/kvm/x86.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index da4bbd043a7b..caa2541833dd 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -6007,8 +6007,11 @@ static int kvm_vm_ioctl_set_nr_mmu_pages(struct kvm *kvm,
> > >   	return 0;
> > >   }
> > > -static unsigned long kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
> > > +static int kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
> > >   {
> > > +	if (kvm->arch.n_max_mmu_pages > INT_MAX)
> > > +		return -EOVERFLOW;
> > > +
> > >   	return kvm->arch.n_max_mmu_pages;
> > >   }
> > 
> > My vote is to skip this patch, skip deprecation, and go straight to deleting
> > KVM_GET_NR_MMU_PAGES.  The ioctl() has never worked[*], and none of the VMMs I
> > checked use it (QEMU, Google's internal VMM, kvmtool, CrosVM).
> 
> I guess I'm living too much in the QEMU world where things need to be
> deprecated first before removing them ;-)

If KVM_GET_NR_MMU_PAGES actually worked or had users then I'd feel differently.
Anything we do to try and make this less awful is going to be an ABI change, so
we might as well go for broke.

> But sure, if everybody agrees that removing this directly is fine, too, I
> can do this in v2.
> 
>  Thomas
> 
> 
> PS: Has there ever been a discussion about the other deprecated interfaces
> in include/uapi/linux/kvm.h ? Most of the stuff there seems to be from 2009
> ... so maybe it's time now to remove that, too?

Not sure.  They aren't actually "deprecated" for most projects' definition of
"deprecated".  AFAICT, "deprecated" here means "removed, but with a placeholder
so that KVM doesn't reuse the old ioctl() number".

It probably makes sense to do the same for KVM_GET_NR_MMU_PAGES.  Yank out the
backing code but leave the ioctl() definition so that if there are users, they
get an explicit error code instead of random behavior, i.e. prevent KVM from
reusing the number in the near future.
diff mbox series

Patch

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index da4bbd043a7b..caa2541833dd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6007,8 +6007,11 @@  static int kvm_vm_ioctl_set_nr_mmu_pages(struct kvm *kvm,
 	return 0;
 }
 
-static unsigned long kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
+static int kvm_vm_ioctl_get_nr_mmu_pages(struct kvm *kvm)
 {
+	if (kvm->arch.n_max_mmu_pages > INT_MAX)
+		return -EOVERFLOW;
+
 	return kvm->arch.n_max_mmu_pages;
 }