| 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
| ||
|
Message-ID: <CAMkAt6r-Mc_YN-gVHuCpTj4E1EmcvyYpP9jhtHo5HRHnoNJAdA@mail.gmail.com>
Date: Fri, 8 Apr 2022 09:08:07 -0600
From: Peter Gonda <pgonda@...gle.com>
To: John Sperbeck <jsperbeck@...gle.com>
Cc: kvm list <kvm@...r.kernel.org>,
David Rientjes <rientjes@...gle.com>,
Sean Christopherson <seanjc@...gle.com>,
Paolo Bonzini <pbonzini@...hat.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] KVM: SEV: Mark nested locking of vcpu->lock
On Thu, Apr 7, 2022 at 3:17 PM John Sperbeck <jsperbeck@...gle.com> wrote:
>
> 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.
Nope. Good catch, I didn't realize there was a limit 8 subclasses:
[ 509.093776] BUG: looking up invalid subclass: 8
[ 509.098314] turning off the locking correctness validator.
[ 509.103800] CPU: 185 PID: 28570 Comm: sev_migrate_tes Tainted: G
O 5.17.0-dbg-DEV #24
[ 509.112925] Hardware name: Google, Inc.
Arcadia_IT_80/Arcadia_IT_80, BIOS
30.6.12-gce 09/27/2021
[ 509.126386] Call Trace:
[ 509.128835] <TASK>
[ 509.130939] dump_stack_lvl+0x6c/0x9a
[ 509.134609] dump_stack+0x10/0x12
[ 509.137925] look_up_lock_class+0xf1/0x130
[ 509.142027] register_lock_class+0x54/0x730
[ 509.146214] __lock_acquire+0x85/0xf00
[ 509.149964] ? lock_is_held_type+0xff/0x170
[ 509.154154] lock_acquire+0xca/0x210
[ 509.157730] ? sev_lock_vcpus_for_migration+0x82/0x150
[ 509.162872] __mutex_lock_common+0xe4/0xe30
[ 509.167054] ? sev_lock_vcpus_for_migration+0x82/0x150
[ 509.172194] ? sev_lock_vcpus_for_migration+0x82/0x150
[ 509.177335] ? rcu_lock_release+0x17/0x20
[ 509.181348] mutex_lock_killable_nested+0x20/0x30
[ 509.186053] sev_lock_vcpus_for_migration+0x82/0x150
[ 509.191019] sev_vm_move_enc_context_from+0x190/0x750
[ 509.196072] ? lock_release+0x20e/0x290
[ 509.199912] kvm_vm_ioctl_enable_cap+0x29d/0x320
[ 509.204531] kvm_vm_ioctl+0xc58/0x1060
[ 509.208285] ? __this_cpu_preempt_check+0x13/0x20
[ 509.212989] __se_sys_ioctl+0x77/0xc0
[ 509.216656] __x64_sys_ioctl+0x1d/0x20
[ 509.220408] do_syscall_64+0x44/0xa0
[ 509.223987] entry_SYSCALL_64_after_hwframe+0x44/0xae
[ 509.229041] RIP: 0033:0x7f91b8531347
[ 509.232618] Code: 5d c3 cc 48 8b 05 f9 2f 07 00 64 c7 00 26 00 00
00 48 c7 c0 ff ff ff ff c3 cc cc cc cc cc cc cc cc cc cc b8 10 00 00
00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c9 2f 07 00 f7 d8 64 89
01 48
[ 509.251371] RSP: 002b:00007ffef7feb778 EFLAGS: 00000246 ORIG_RAX:
0000000000000010
[ 509.258940] RAX: ffffffffffffffda RBX: 0000000000af6210 RCX: 00007f91b8531347
[ 509.266073] RDX: 00007ffef7feb790 RSI: 000000004068aea3 RDI: 0000000000000018
[ 509.273207] RBP: 00007ffef7feba10 R08: 000000000020331b R09: 000000000000000f
[ 509.280338] R10: 0000000000000001 R11: 0000000000000246 R12: 0000000000af8df0
[ 509.287470] R13: 0000000000afa3e0 R14: 0000000000000000 R15: 0000000000af7800
[ 509.294607] </TASK>
> >
> > 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?
Powered by blists - more mailing lists