On Thu, Oct 10, 2024, Paolo Bonzini wrote:
> On 10/9/24 17:04, Sean Christopherson wrote:
> > Now that KVM loads from vcpu_array if and only if the target index is
> > valid with respect to online_vcpus, i.e. now that it is safe to erase a
> > not-fully-onlined vCPU entry, revert to storing into vcpu_array before
> > success is guaranteed.
> >
> > If xa_store() fails, which _should_ be impossible, then putting the vCPU's
> > reference to 'struct kvm' results in a refcounting bug as the vCPU fd has
> > been installed and owns the vCPU's reference.
> >
> > This was found by inspection, but forcing the xa_store() to fail
> > confirms the problem:
> >
> > | Unable to handle kernel paging request at virtual address ffff800080ecd960
> > | Call trace:
> > | _raw_spin_lock_irq+0x2c/0x70
> > | kvm_irqfd_release+0x24/0xa0
> > | kvm_vm_release+0x1c/0x38
> > | __fput+0x88/0x2ec
> > | ____fput+0x10/0x1c
> > | task_work_run+0xb0/0xd4
> > | do_exit+0x210/0x854
> > | do_group_exit+0x70/0x98
> > | get_signal+0x6b0/0x73c
> > | do_signal+0xa4/0x11e8
> > | do_notify_resume+0x60/0x12c
> > | el0_svc+0x64/0x68
> > | el0t_64_sync_handler+0x84/0xfc
> > | el0t_64_sync+0x190/0x194
> > | Code: b9000909 d503201f 2a1f03e1 52800028 (88e17c08)
> >
> > Practically speaking, this is a non-issue as xa_store() can't fail, absent
> > a nasty kernel bug. But the code is visually jarring and technically
> > broken.
> >
> > This reverts commit afb2acb2e3a32e4d56f7fbd819769b98ed1b7520.
> >
> > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Cc: Michal Luczaj <mhal@xxxxxxx>
> > Cc: Alexander Potapenko <glider@xxxxxxxxxx>
> > Cc: Marc Zyngier <maz@xxxxxxxxxx>
> > Reported-by: Will Deacon <will@xxxxxxxxxx>
> > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > ---
> > virt/kvm/kvm_main.c | 14 +++++---------
> > 1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index fca9f74e9544..f081839521ef 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -4283,7 +4283,8 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
> > }
> > vcpu->vcpu_idx = atomic_read(&kvm->online_vcpus);
> > - r = xa_reserve(&kvm->vcpu_array, vcpu->vcpu_idx, GFP_KERNEL_ACCOUNT);
> > + r = xa_insert(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, GFP_KERNEL_ACCOUNT);
> > + BUG_ON(r == -EBUSY);
> > if (r)
> > goto unlock_vcpu_destroy;
> > @@ -4298,12 +4299,7 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, unsigned long id)
> > kvm_get_kvm(kvm);
> > r = create_vcpu_fd(vcpu);
> > if (r < 0)
> > - goto kvm_put_xa_release;
> > -
> > - if (KVM_BUG_ON(xa_store(&kvm->vcpu_array, vcpu->vcpu_idx, vcpu, 0), kvm)) {
> > - r = -EINVAL;
> > - goto kvm_put_xa_release;
> > - }
> > + goto kvm_put_xa_erase;
>
> I also find it a bit jarring though that we have to undo the insertion. This
> is a chicken-and-egg situation where you are pick one operation B that will
> have to undo operation A if it fails. But what xa_store is doing, is
> breaking this deadlock.
>
> The code is a bit longer, sure, but I don't see the point in complicating
> the vcpu_array invariants and letting an entry disappear.
But we only need one rule: vcpu_array[x] is valid if and only if 'x' is less than
online_vcpus. And that rule is necessary regardless of whether or not vcpu_array[x]
is filled before success is guaranteed.
I'm not concerned about the code length, it's that we need to do _something_ if
xa_store() fails. Yeah, it should never happen, but knowingly doing nothing feels
all kinds of wrong. I don't like BUG(), because it's obviously very doable to
gracefully handle failure. And a WARN() is rather pointless, because continuing
on with an invalid entry is all but guaranteed to crash, i.e. is little more than a
deferred BUG() in this case.
