Re: [PATCH 10/12] KVM: SEV: Prohibit migration of a VM that has mirrors

[Paolo Bonzini]

On 12/1/21 19:17, Peter Gonda wrote:
> On Mon, Nov 22, 2021 at 5:50 PM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
> > VMs that mirror an encryption context rely on the owner to keep the
> > ASID allocated.  Performing a KVM_CAP_VM_MOVE_ENC_CONTEXT_FROM
> > would cause a dangling ASID:
> >
> > 1. copy context from A to B (gets ref to A)
> > 2. move context from A to L (moves ASID from A to L)
> > 3. close L (releases ASID from L, B still references it)
> >
> > The right way to do the handoff instead is to create a fresh mirror VM
> > on the destination first:
> >
> > 1. copy context from A to B (gets ref to A)
> > [later] 2. close B (releases ref to A)
> > 3. move context from A to L (moves ASID from A to L)
> > 4. copy context from L to M
> >
> > So, catch the situation by adding a count of how many VMs are
> > mirroring this one's encryption context.
> >
> > Fixes: 0b020f5af092 ("KVM: SEV: Add support for SEV-ES intra host migration")
> > Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > ---
> >   arch/x86/kvm/svm/sev.c                        | 22 ++++++++++-
> >   arch/x86/kvm/svm/svm.h                        |  1 +
> >   .../selftests/kvm/x86_64/sev_migrate_tests.c  | 37 +++++++++++++++++++
> >   3 files changed, 59 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index 025d9731b66c..89a716290fac 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -1696,6 +1696,16 @@ int svm_vm_migrate_from(struct kvm *kvm, unsigned int source_fd)
> >          }
> >
> >          src_sev = &to_kvm_svm(source_kvm)->sev_info;
> > +
> > +       /*
> > +        * VMs mirroring src's encryption context rely on it to keep the
> > +        * ASID allocated, but below we are clearing src_sev->asid.
> > +        */
> > +       if (src_sev->num_mirrored_vms) {
> > +               ret = -EBUSY;
> > +               goto out_unlock;
> > +       }
> > +
> >          dst_sev->misc_cg = get_current_misc_cg();
> >          cg_cleanup_sev = dst_sev;
> >          if (dst_sev->misc_cg != src_sev->misc_cg) {
> > @@ -1987,6 +1997,7 @@ int svm_vm_copy_asid_from(struct kvm *kvm, unsigned int source_fd)
> >           */
> >          source_sev = &to_kvm_svm(source_kvm)->sev_info;
> >          kvm_get_kvm(source_kvm);
> > +       source_sev->num_mirrored_vms++;
> >
> >          /* Set enc_context_owner and copy its encryption context over */
> >          mirror_sev = &to_kvm_svm(kvm)->sev_info;
> > @@ -2019,12 +2030,21 @@ void sev_vm_destroy(struct kvm *kvm)
> >          struct list_head *head = &sev->regions_list;
> >          struct list_head *pos, *q;
> >
> > +       WARN_ON(sev->num_mirrored_vms);
> > +
>
> If we don't change to atomic doesn't this need to happen when we have
> the kvm->lock?

Hi,

there can be no concurrency when destroying a VM.  (If you do Rust, it's 
similar to using x.get_mut().get_mut() on an Arc<Mutex<T>>).

Paolo