On Wed, Apr 28, 2021 at 02:38:57PM +0200, Vitaly Kuznetsov wrote:
> Valeriy Vdovin <valeriy.vdovin@xxxxxxxxxxxxx> writes:
>
> > KVM_GET_CPUID2 kvm ioctl is not very well documented, but the way it is
> > implemented in function kvm_vcpu_ioctl_get_cpuid2 suggests that even at
> > error path it will try to return number of entries to the caller. But
> > The dispatcher kvm vcpu ioctl dispatcher code in kvm_arch_vcpu_ioctl
> > ignores any output from this function if it sees the error return code.
> >
> > It's very explicit by the code that it was designed to receive some
> > small number of entries to return E2BIG along with the corrected number.
> >
> > This lost logic in the dispatcher code has been restored by removing the
> > lines that check for function return code and skip if error is found.
> > Without it, the ioctl caller will see both the number of entries and the
> > correct error.
> >
> > In selftests relevant function vcpu_get_cpuid has also been modified to
> > utilize the number of cpuid entries returned along with errno E2BIG.
> >
> > Signed-off-by: Valeriy Vdovin <valeriy.vdovin@xxxxxxxxxxxxx>
> > ---
> > arch/x86/kvm/x86.c | 10 +++++-----
> > .../selftests/kvm/lib/x86_64/processor.c | 20 +++++++++++--------
> > 2 files changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index efc7a82ab140..df8a3e44e722 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -4773,14 +4773,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
> > 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)))
>
> It may make sense to check that 'r == -E2BIG' before trying to write
> anything back. I don't think it is correct/expected to modify nent in
> other cases (e.g. when kvm_vcpu_ioctl_get_cpuid2() returns -EFAULT)
>
That's a good point. The caller could expect and rely on the fact that nent
is unmodified in any error case except E2BIG. I will add this in the next
version.
> > +
> > + if (copy_to_user(cpuid_arg, &cpuid, sizeof(cpuid))) {
> > + r = -EFAULT;
> > goto out;
> > - r = 0;
> > + }
> > break;
>
> How is KVM userspace supposed to know if it can trust the 'nent' value
> (KVM is fixed case) or not (KVM is not fixed case)? This can probably be
> resolved with adding a new capability (but then I'm not sure the change
> is worth it to be honest).
As I see it KVM userspace should set nent to 0, and then expect any non-zero
value in return along with E2BIG. This is the same approach I've used in the
modified test code in the same patch.
> Also, if making such a change, API
> documentation in virt/kvm/api.rst needs updating.
Of course. I will add changes to the documentation and comments in case if this
change in general will have a go.
>
> > }
> > case KVM_GET_MSRS: {
> > diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > index a8906e60a108..a412b39ad791 100644
> > --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> > @@ -727,17 +727,21 @@ struct kvm_cpuid2 *vcpu_get_cpuid(struct kvm_vm *vm, uint32_t vcpuid)
> >
> > cpuid = allocate_kvm_cpuid2();
> > max_ent = cpuid->nent;
> > + cpuid->nent = 0;
> >
> > - for (cpuid->nent = 1; cpuid->nent <= max_ent; cpuid->nent++) {
> > - rc = ioctl(vcpu->fd, KVM_GET_CPUID2, cpuid);
> > - if (!rc)
> > - break;
> > + rc = ioctl(vcpu->fd, KVM_GET_CPUID2, cpuid);
> > + TEST_ASSERT(rc == -1 && errno == E2BIG,
> > + "KVM_GET_CPUID2 should return E2BIG: %d %d",
> > + rc, errno);
> >
> > - TEST_ASSERT(rc == -1 && errno == E2BIG,
> > - "KVM_GET_CPUID2 should either succeed or give E2BIG: %d %d",
> > - rc, errno);
> > - }
> > + TEST_ASSERT(cpuid->nent,
> > + "KVM_GET_CPUID2 failed to set cpuid->nent with E2BIG");
> > +
> > + TEST_ASSERT(cpuid->nent < max_ent,
> > + "KVM_GET_CPUID2 has %d entries, expected maximum: %d",
> > + cpuid->nent, max_ent);
> >
> > + rc = ioctl(vcpu->fd, KVM_GET_CPUID2, cpuid);
> > TEST_ASSERT(rc == 0, "KVM_GET_CPUID2 failed, rc: %i errno: %i",
> > rc, errno);
>
> --
> Vitaly
>
