Re: [RFC PATCH v3 08/11] KVM: Fix set_mem_attr ioctl when error case

[Sean Christopherson]

On Fri, Jul 14, 2023, Zhi Wang wrote:
> On Thu, 13 Jul 2023 15:03:54 -0700
> Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > +       /*
> > +        * Reserve memory ahead of time to avoid having to deal with failures
> > +        * partway through setting the new attributes.
> > +        */
> > +       for (i = start; i < end; i++) {
> > +               r = xa_reserve(&kvm->mem_attr_array, i, GFP_KERNEL_ACCOUNT);
> > +               if (r)
> > +                       goto out_unlock;
> > +       }
> > +
> >         KVM_MMU_LOCK(kvm);
> >         kvm_mmu_invalidate_begin(kvm);
> >         kvm_mmu_invalidate_range_add(kvm, start, end);
> >         KVM_MMU_UNLOCK(kvm);
> >  
> >         for (i = start; i < end; i++) {
> > -               if (xa_err(xa_store(&kvm->mem_attr_array, i, entry,
> > -                                   GFP_KERNEL_ACCOUNT)))
> > +               r = xa_err(xa_store(&kvm->mem_attr_array, i, entry,
> > +                                   GFP_KERNEL_ACCOUNT));
> > +               if (KVM_BUG_ON(r, kvm))
> >                         break;
> >         }
> >
> 
> IIUC, If an error happenes here, we should bail out and call xa_release()?
> Or the code below (which is not shown here) still changes the memory attrs
> partially.

I'm pretty sure we want to continue on.  The VM is dead (killed by KVM_BUG_ON()),
so the attributes as seen by userspace and/or the VM don't matter.  What does
matter is that KVM's internal state is consistent, e.g. that KVM doesn't have
shared SPTEs while the attributes say a GFN is private.  That might not matter
for teardown, but I can't think of any reason not to tidy up.

And there can also be other ioctls() in flight.  KVM_REQ_VM_DEAD ensures vCPU
can't enter the guest, and vm->vm_dead ensures no new ioctls() cant start, but
neither of those guarantees there aren't other tasks doing KVM things.

Regardless, we definitely don't need to do xa_release().  The VM is dead and all
its memory will be reclaimed soon enough.  And there's no guarantee xa_release()
will actually be able to free anything, e.g. already processed entries won't be
freed, nor will any entries that existed _before_ the ioctl() was invoked.  Not
to mention that the xarray probably isn't consuming much memory, relatively
speaking.  I.e. in the majority of scenarios, it's likely preferable to get out
and destroy the VM asap.