Re: [PATCH 4/6] Revert "KVM: Fix vcpu_array[0] races"

[Sean Christopherson]

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.