linux-kernel - Re: [PATCH v3] KVM: SEV: Mark nested locking of vcpu->lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFNjLiXC0AdOw5f8Ovu47D==ex7F0=WN_Ocirymz4xL=mWvC5A@mail.gmail.com>
Date:   Thu, 7 Apr 2022 14:17:32 -0700
From:   John Sperbeck <jsperbeck@...gle.com>
To:     Peter Gonda <pgonda@...gle.com>
Cc:     kvm@...r.kernel.org, David Rientjes <rientjes@...gle.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] KVM: SEV: Mark nested locking of vcpu->lock

On Thu, Apr 7, 2022 at 12:59 PM Peter Gonda <pgonda@...gle.com> wrote:
>
> svm_vm_migrate_from() uses sev_lock_vcpus_for_migration() to lock all
> source and target vcpu->locks. Mark the nested subclasses to avoid false
> positives from lockdep.
>
> Warning example:
> ============================================
> WARNING: possible recursive locking detected
> 5.17.0-dbg-DEV #15 Tainted: G           O
> --------------------------------------------
> sev_migrate_tes/18859 is trying to acquire lock:
> ffff8d672d484238 (&vcpu->mutex){+.+.}-{3:3}, at: sev_lock_vcpus_for_migration+0x7e/0x150
> but task is already holding lock:
> ffff8d67703f81f8 (&vcpu->mutex){+.+.}-{3:3}, at: sev_lock_vcpus_for_migration+0x7e/0x150
> other info that might help us debug this:
>  Possible unsafe locking scenario:
>        CPU0
>        ----
>   lock(&vcpu->mutex);
>   lock(&vcpu->mutex);
>  *** DEADLOCK ***
>  May be due to missing lock nesting notation
> 3 locks held by sev_migrate_tes/18859:
>  #0: ffff9302f91323b8 (&kvm->lock){+.+.}-{3:3}, at: sev_vm_move_enc_context_from+0x96/0x740
>  #1: ffff9302f906a3b8 (&kvm->lock/1){+.+.}-{3:3}, at: sev_vm_move_enc_context_from+0xae/0x740
>  #2: ffff8d67703f81f8 (&vcpu->mutex){+.+.}-{3:3}, at: sev_lock_vcpus_for_migration+0x7e/0x150
>
> Fixes: b56639318bb2b ("KVM: SEV: Add support for SEV intra host migration")
> Reported-by: John Sperbeck<jsperbeck@...gle.com>
> Suggested-by: David Rientjes <rientjes@...gle.com>
> Suggested-by: Sean Christopherson <seanjc@...gle.com>
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: kvm@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Signed-off-by: Peter Gonda <pgonda@...gle.com>
>
> ---
>
> V3
>  * Updated signature to enum to self-document argument.
>  * Updated comment as Seanjc@ suggested.
>
> Tested by running sev_migrate_tests with lockdep enabled. Before we see
> a warning from sev_lock_vcpus_for_migration(). After we get no warnings.
>
> ---
>  arch/x86/kvm/svm/sev.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 75fa6dd268f0..f66550ec8eaf 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -1591,14 +1591,26 @@ static void sev_unlock_two_vms(struct kvm *dst_kvm, struct kvm *src_kvm)
>         atomic_set_release(&src_sev->migration_in_progress, 0);
>  }
>
> +/*
> + * To suppress lockdep false positives, subclass all vCPU mutex locks by
> + * assigning even numbers to the source vCPUs and odd numbers to destination
> + * vCPUs based on the vCPU's index.
> + */
> +enum sev_migration_role {
> +       SEV_MIGRATION_SOURCE = 0,
> +       SEV_MIGRATION_TARGET,
> +       SEV_NR_MIGRATION_ROLES,
> +};
>
> -static int sev_lock_vcpus_for_migration(struct kvm *kvm)
> +static int sev_lock_vcpus_for_migration(struct kvm *kvm,
> +                                       enum sev_migration_role role)
>  {
>         struct kvm_vcpu *vcpu;
>         unsigned long i, j;
>
> -       kvm_for_each_vcpu(i, vcpu, kvm) {
> -               if (mutex_lock_killable(&vcpu->mutex))
> +       kvm_for_each_vcpu(i, vcpu, kvm) {
> +               if (mutex_lock_killable_nested(
> +                           &vcpu->mutex, i * SEV_NR_MIGRATION_ROLES + role))
>                         goto out_unlock;
>         }
>
> @@ -1745,10 +1757,10 @@ int sev_vm_move_enc_context_from(struct kvm *kvm, unsigned int source_fd)
>                 charged = true;
>         }
>
> -       ret = sev_lock_vcpus_for_migration(kvm);
> +       ret = sev_lock_vcpus_for_migration(kvm, SEV_MIGRATION_SOURCE);
>         if (ret)
>                 goto out_dst_cgroup;
> -       ret = sev_lock_vcpus_for_migration(source_kvm);
> +       ret = sev_lock_vcpus_for_migration(source_kvm, SEV_MIGRATION_TARGET);
>         if (ret)
>                 goto out_dst_vcpu;
>
> --
> 2.35.1.1178.g4f1659d476-goog
>

Does sev_migrate_tests survive lockdep checking if
NR_MIGRATE_TEST_VCPUS is changed to 16?