On Thu, Jun 06, 2024, Mathias Krause wrote:
> On 06.06.24 16:55, Sean Christopherson wrote:
> > On Thu, Jun 06, 2024, Mathias Krause wrote:
> The first part is _completely_ handled by the 'id >= KVM_MAX_VCPU_IDS'
> test, as 'id' is still the "raw" value userland provided.
I'm not arguing it doesn't, all I'm saying is that I don't like overloading a
check against an arbitrary limit to also protect against truncation issues.
> > E.g. x86 has another potentially more restrictive check on @id, and it looks
> > quite odd to check @id against KVM_MAX_VCPU_IDS as an "unsigned long" in flow
> > flow, but as an "unsigned int" in another.
>
> Again, that's two distinct things, even if looking similar. The first
> check against KVM_MAX_VCPU_IDS does actually two things:
>
> 1/ Ensure the full user ABI provided value (ulong) is sane and
> 2/ ensure it's within the hard limits KVM expects (fits unsigned int).
>
> Now we do both with only a single compare and maybe that's what's so
> hard to grasp -- that a single check can do both things. But why not
> make use of simple things when we can do so?
Because it's unnecessarily clever. I've dealt with waaaay too much legacy KVM
code that probably seemed super obvious at the time, but 8+ years and lots of
code churn later was completely nonsensical and all but impossible to decipher.
That's less likely to happen these days, as we have better tracking via lore, and
I like to think we have better changelogs. But I still dislike doing unnecessarily
clever things because it tends to set the next generation up to fail.
> >> --- a/virt/kvm/kvm_main.c
> >> +++ b/virt/kvm/kvm_main.c
> >> @@ -4200,12 +4200,13 @@ static void kvm_create_vcpu_debugfs(struct
> >> kvm_vcpu *vcpu)
> >> /*
> >> * Creates some virtual cpus. Good luck creating more than one.
> >> */
> >> -static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
> >> +static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
> >> {
> >> int r;
> >> struct kvm_vcpu *vcpu;
> >> struct page *page;
> >>
> >> + BUILD_BUG_ON(KVM_MAX_VCPU_IDS > INT_MAX);
> >
> > This should be UINT_MAX, no?
>
> No, I chose INT_MAX very intentional, as the underlying type of
> 'vcpu_id' is actually an int.
Oof, I didn't realize (or more likely, simply forgot) that vcpu_id and vcpu_idx
are tracked as "int".
> There's no "need" for the BUILD_BUG_ON(). It's just a cheap (compile
> time only, no runtime "overhead") assert that the code won't allow
> truncated values which may lead to follow-up bugs because of unintended
> truncation. And, after all, you suggested something like that (a
> truncation check) yourself. I just tried to provide it as something that
> doesn't need the odd '__id' argument and an explicit truncation check
> which would do the wrong thing if we would like to push KVM_MAX_VCPU_IDS
> above UINT_MAX (failing only at runtime, not at compile time).
Yeah, I know all that. I'm not arguing the actual cost of the code is at all
meaningful. I'm purely concerned about the long-term maintenance cost. This is
obviously a small thing that is unlikely to ever cause problems, but again, I
suspect that past KVM developers said exactly that about things that have pegged
my WTF-o-meter.
> > If @id is checked as a 32-bit value, and we somehow screw up and
> > define KVM_MAX_VCPU_IDS to be a 64-bit value, clang will rightly complain that
> > the check is useless, e.g. given "#define KVM_MAX_VCPU_ID_TEST BIT(32)"
> >
> > arch/x86/kvm/x86.c:12171:9: error: result of comparison of constant 4294967296 with
> > expression of type 'unsigned int' is always false [-Werror,-Wtautological-constant-out-of-range-compare]
> > if (id > KVM_MAX_VCPU_ID_TEST)
> > ~~ ^ ~~~~~~~~~~~~~~~~~~~~
> > 1 error generated.
> ^^^^^^^^^^^^^^^^^^
> Perfect! So this breaks the build. How much better can we prevent this
> bug from going unnoticed?
Yes, but iff @id is a 32-bit value, i.e. this trick doesn't work on 64-bit kernels
if the comparison is done with @id is an unsigned long (and I'm hoping that we
can kill off 32-bit KVM support in the not too distant future).
> ¹ IMHO, using 'int' for vcpu_id is actually *very* *wrong*, as it's used
> as an index in certain constructs and having a signed type doesn't feel
> right at all. But that's just a side matter, as, according to the checks
> on the ioctl() path, the actual value of vcpu_id can never be negative.
> So lets not distract.
Hmm, I 100% agree that it's horrific, but I disagree that it's a distraction.
I think we should fix that at the same time as we harden the trunction stuff, so
that it's (hopefully) clear what KVM _intends_ to support, as opposed to what the
code happens to allow.
In the end, I'm ok relying on the KVM_MAX_VCPU_IDS check, so long as there's
a BUILD_BUG_ON() and a comment.
