[PATCH 1/2] KVM: Don't fail KVM_GET_SUPPORTED_CPUID if nent is just right

[PATCH 1/2] KVM: Don't fail KVM_GET_SUPPORTED_CPUID if nent is just right

[Sasha Levin]

If we pass just enough entries to KVM_GET_SUPPORTED_CPUID, we would still
fail with -E2BIG due to wrong comparisons.

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 arch/x86/kvm/x86.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 9eff4af..460c49b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2664,7 +2664,7 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
 		do_cpuid_ent(&cpuid_entries[nent], func, 0,
 			     &nent, cpuid->nent);
 	r = -E2BIG;
-	if (nent >= cpuid->nent)
+	if (nent > cpuid->nent)
 		goto out_free;
 
 	do_cpuid_ent(&cpuid_entries[nent], 0x80000000, 0, &nent, cpuid->nent);
@@ -2676,7 +2676,7 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
 
 
 	r = -E2BIG;
-	if (nent >= cpuid->nent)
+	if (nent > cpuid->nent)
 		goto out_free;
 
 	/* Add support for Centaur's CPUID instruction. */
@@ -2685,7 +2685,7 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
 				&nent, cpuid->nent);
 
 		r = -E2BIG;
-		if (nent >= cpuid->nent)
+		if (nent > cpuid->nent)
 			goto out_free;
 
 		limit = cpuid_entries[nent - 1].eax;
@@ -2695,7 +2695,7 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
 					&nent, cpuid->nent);
 
 		r = -E2BIG;
-		if (nent >= cpuid->nent)
+		if (nent > cpuid->nent)
 			goto out_free;
 	}
 
@@ -2703,14 +2703,14 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
 		     cpuid->nent);
 
 	r = -E2BIG;
-	if (nent >= cpuid->nent)
+	if (nent > cpuid->nent)
 		goto out_free;
 
 	do_cpuid_ent(&cpuid_entries[nent], KVM_CPUID_FEATURES, 0, &nent,
 		     cpuid->nent);
 
 	r = -E2BIG;
-	if (nent >= cpuid->nent)
+	if (nent > cpuid->nent)
 		goto out_free;
 
 	r = -EFAULT;
-- 
1.7.8.rc1

[PATCH 2/2] KVM: Correct documentation of KVM_GET_SUPPORTED_CPUID

[Sasha Levin]

If the amount of entries available passed to KVM_GET_SUPPORTED_CPUID is
too big we don't fail, we just adjust it to the amount actually needed
and fill the entries.

Cc: Avi Kivity <avi@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 Documentation/virtual/kvm/api.txt |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 7945b0b..273be09 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -1074,10 +1074,9 @@ or for feature consistency across a cluster).
 Userspace invokes KVM_GET_SUPPORTED_CPUID by passing a kvm_cpuid2 structure
 with the 'nent' field indicating the number of entries in the variable-size
 array 'entries'.  If the number of entries is too low to describe the cpu
-capabilities, an error (E2BIG) is returned.  If the number is too high,
-the 'nent' field is adjusted and an error (ENOMEM) is returned.  If the
-number is just right, the 'nent' field is adjusted to the number of valid
-entries in the 'entries' array, which is then filled.
+capabilities, an error (E2BIG) is returned.  If the number is above or just,
+right, the 'nent' field is adjusted to the number of valid entries in the
+'entries' array, which is then filled.
 
 The entries returned are the host cpuid as returned by the cpuid instruction,
 with unknown or unsupported features masked out.  Some features (for example,
-- 
1.7.8.rc1

Re: [PATCH 2/2] KVM: Correct documentation of KVM_GET_SUPPORTED_CPUID

[Sasha Levin]

Avi,

This is the other part of the get_supported_cpuid change. We discussed
it over IRC and you said it looks right.

On Thu, 2011-11-17 at 12:18 +0200, Sasha Levin wrote:
> If the amount of entries available passed to KVM_GET_SUPPORTED_CPUID is
> too big we don't fail, we just adjust it to the amount actually needed
> and fill the entries.
> 
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  Documentation/virtual/kvm/api.txt |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 7945b0b..273be09 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -1074,10 +1074,9 @@ or for feature consistency across a cluster).
>  Userspace invokes KVM_GET_SUPPORTED_CPUID by passing a kvm_cpuid2 structure
>  with the 'nent' field indicating the number of entries in the variable-size
>  array 'entries'.  If the number of entries is too low to describe the cpu
> -capabilities, an error (E2BIG) is returned.  If the number is too high,
> -the 'nent' field is adjusted and an error (ENOMEM) is returned.  If the
> -number is just right, the 'nent' field is adjusted to the number of valid
> -entries in the 'entries' array, which is then filled.
> +capabilities, an error (E2BIG) is returned.  If the number is above or just,
> +right, the 'nent' field is adjusted to the number of valid entries in the
> +'entries' array, which is then filled.
>  
>  The entries returned are the host cpuid as returned by the cpuid instruction,
>  with unknown or unsupported features masked out.  Some features (for example,

-- 

Sasha.

Re: [PATCH 1/2] KVM: Don't fail KVM_GET_SUPPORTED_CPUID if nent is just right

[Marcelo Tosatti]

On Thu, Nov 17, 2011 at 12:18:44PM +0200, Sasha Levin wrote:
> If we pass just enough entries to KVM_GET_SUPPORTED_CPUID, we would still
> fail with -E2BIG due to wrong comparisons.
> 
> Cc: Avi Kivity <avi@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  arch/x86/kvm/x86.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 9eff4af..460c49b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2664,7 +2664,7 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
>  		do_cpuid_ent(&cpuid_entries[nent], func, 0,
>  			     &nent, cpuid->nent);
>  	r = -E2BIG;
> -	if (nent >= cpuid->nent)
> +	if (nent > cpuid->nent)
>  		goto out_free;

"int nent" variable contains the index into the array. 
"__u32 cpuid->nent", from userspace, contains the number
of entries in the array.

So the ">=" comparison is necessary to avoid overwriting past the end of
the array.

The protocol goes like "try size x, if it fails with -E2BIG, increase x,
try again". Its awkward.

Re: [PATCH 1/2] KVM: Don't fail KVM_GET_SUPPORTED_CPUID if nent is just right

[Sasha Levin]

On Thu, 2011-11-24 at 08:09 -0200, Marcelo Tosatti wrote:
> On Thu, Nov 17, 2011 at 12:18:44PM +0200, Sasha Levin wrote:
> > If we pass just enough entries to KVM_GET_SUPPORTED_CPUID, we would still
> > fail with -E2BIG due to wrong comparisons.
> > 
> > Cc: Avi Kivity <avi@redhat.com>
> > Cc: Marcelo Tosatti <mtosatti@redhat.com>
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > ---
> >  arch/x86/kvm/x86.c |   12 ++++++------
> >  1 files changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 9eff4af..460c49b 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -2664,7 +2664,7 @@ static int kvm_dev_ioctl_get_supported_cpuid(struct kvm_cpuid2 *cpuid,
> >  		do_cpuid_ent(&cpuid_entries[nent], func, 0,
> >  			     &nent, cpuid->nent);
> >  	r = -E2BIG;
> > -	if (nent >= cpuid->nent)
> > +	if (nent > cpuid->nent)
> >  		goto out_free;
> 
> "int nent" variable contains the index into the array. 
> "__u32 cpuid->nent", from userspace, contains the number
> of entries in the array.
> 
> So the ">=" comparison is necessary to avoid overwriting past the end of
> the array.

Right, only the last comparison should be changed to ">" because in that
case It's ok if the nent (which points to the next entry) equals to
cpuid->nent.

> 
> The protocol goes like "try size x, if it fails with -E2BIG, increase x,
> try again". Its awkward.

We can set nent to be the amount of entries required like we do in the
opposite case where we passed too many entries.

-- 

Sasha.

Re: [PATCH 1/2] KVM: Don't fail KVM_GET_SUPPORTED_CPUID if nent is just right

[Avi Kivity]

On 11/24/2011 12:31 PM, Sasha Levin wrote:
> > 
> > The protocol goes like "try size x, if it fails with -E2BIG, increase x,
> > try again". Its awkward.
>
> We can set nent to be the amount of entries required like we do in the
> opposite case where we passed too many entries.

There's no point, since userspace will want to support older kernels.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] KVM: Don't fail KVM_GET_SUPPORTED_CPUID if nent is just right

[Sasha Levin]

On Thu, 2011-11-24 at 12:33 +0200, Avi Kivity wrote:
> On 11/24/2011 12:31 PM, Sasha Levin wrote:
> > > 
> > > The protocol goes like "try size x, if it fails with -E2BIG, increase x,
> > > try again". Its awkward.
> >
> > We can set nent to be the amount of entries required like we do in the
> > opposite case where we passed too many entries.
> 
> There's no point, since userspace will want to support older kernels.

In the case of old kernels the cpuid->nent value will not be modified,
so userspace can handle both cases easily:

- If KVM_GET_SUPPORTED_CPUID returned -E2BIG, check cpuid->nent
	- If zero, do same -E2BIG loop as we do now.
	- If not, allocate amount needed and pass it to the ioctl again.

-- 

Sasha.

Re: [PATCH 1/2] KVM: Don't fail KVM_GET_SUPPORTED_CPUID if nent is just right

[Avi Kivity]

On 11/24/2011 12:37 PM, Sasha Levin wrote:
> On Thu, 2011-11-24 at 12:33 +0200, Avi Kivity wrote:
> > On 11/24/2011 12:31 PM, Sasha Levin wrote:
> > > > 
> > > > The protocol goes like "try size x, if it fails with -E2BIG, increase x,
> > > > try again". Its awkward.
> > >
> > > We can set nent to be the amount of entries required like we do in the
> > > opposite case where we passed too many entries.
> > 
> > There's no point, since userspace will want to support older kernels.
>
> In the case of old kernels the cpuid->nent value will not be modified,
> so userspace can handle both cases easily:
>
> - If KVM_GET_SUPPORTED_CPUID returned -E2BIG, check cpuid->nent
> 	- If zero, do same -E2BIG loop as we do now.
> 	- If not, allocate amount needed and pass it to the ioctl again.
>

What's the point?  The code becomes more complicated.

Something like 'while (try_get_cpuid(x) == -E2BIG) { x *= 2; }' is
simple and works everywhere.

-- 
error compiling committee.c: too many arguments to function