From: Mingwei Zhang <mizhang@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Sean Christopherson <seanjc@google.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>,
Wanpeng Li <wanpengli@tencent.com>,
Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
Ben Gardon <bgardon@google.com>,
David Matlack <dmatlack@google.com>,
Mingwei Zhang <mizhang@google.com>
Subject: [PATCH] KVM: x86/mmu: add lockdep check before lookup_address_in_mm()
Date: Sun, 27 Mar 2022 20:58:03 +0000 [thread overview]
Message-ID: <20220327205803.739336-1-mizhang@google.com> (raw)
Add a lockdep check before invoking lookup_address_in_mm().
lookup_address_in_mm() walks all levels of host page table without
accquiring any lock. This is usually unsafe unless we are walking the
kernel addresses (check other usage cases of lookup_address_in_mm and
lookup_address_in_pgd).
Walking host page table (especially guest addresses) usually requires
holding two types of locks: 1) mmu_lock in mm or the lock that protects
the reverse maps of host memory in range; 2) lock for the leaf paging
structures.
One exception case is when we take the mmu_lock of the secondary mmu.
Holding mmu_lock of KVM MMU in either read mode or write mode prevents host
level entities from modifying the host page table concurrently. This is
because all of them will have to invoke KVM mmu_notifier first before doing
the actual work. Since KVM mmu_notifier invalidation operations always take
the mmu write lock, we are safe if we hold the mmu lock here.
Note: this means that KVM cannot allow concurrent multiple mmu_notifier
invalidation callbacks by using KVM mmu read lock. Since, otherwise, any
host level entity can cause race conditions with this one. Walking host
page table here may get us stale information or may trigger NULL ptr
dereference that is hard to reproduce.
Having a lockdep check here will prevent or at least warn future
development that directly walks host page table simply in a KVM ioctl
function. In addition, it provides a record for any future development on
KVM mmu_notifier.
Cc: Sean Christopherson <seanjc@google.com>
Cc: Ben Gardon <bgardon@google.com>
Cc: David Matlack <dmatlack@google.com>
Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
arch/x86/kvm/mmu/mmu.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 1361eb4599b4..066bb5435156 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2820,6 +2820,24 @@ static int host_pfn_mapping_level(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
*/
hva = __gfn_to_hva_memslot(slot, gfn);
+ /*
+ * lookup_address_in_mm() walks all levels of host page table without
+ * accquiring any lock. This is not safe when KVM does not take the
+ * mmu_lock. Holding mmu_lock in either read mode or write mode prevents
+ * host level entities from modifying the host page table. This is
+ * because all of them will have to invoke KVM mmu_notifier first before
+ * doing the actual work. Since KVM mmu_notifier invalidation operations
+ * always take the mmu write lock, we are safe if we hold the mmu lock
+ * here.
+ *
+ * Note: this means that KVM cannot allow concurrent multiple
+ * mmu_notifier invalidation callbacks by using KVM mmu read lock.
+ * Otherwise, any host level entity can cause race conditions with this
+ * one. Walking host page table here may get us stale information or may
+ * trigger NULL ptr dereference that is hard to reproduce.
+ */
+ lockdep_assert_held(&kvm->mmu_lock);
+
pte = lookup_address_in_mm(kvm->mm, hva, &level);
if (unlikely(!pte))
return PG_LEVEL_4K;
--
2.35.1.1021.g381101b075-goog
next reply other threads:[~2022-03-27 20:58 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-27 20:58 Mingwei Zhang [this message]
2022-03-28 15:16 ` [PATCH] KVM: x86/mmu: add lockdep check before lookup_address_in_mm() Sean Christopherson
2022-03-28 17:41 ` Mingwei Zhang
2022-03-28 18:15 ` Sean Christopherson
2022-04-26 17:19 ` Mingwei Zhang
2022-04-26 17:49 ` Paolo Bonzini
2022-04-26 18:10 ` Sean Christopherson
2022-04-26 18:48 ` Mingwei Zhang
2022-04-27 1:16 ` Sean Christopherson
2022-04-27 1:24 ` Mingwei Zhang
2022-04-27 1:30 ` Sean Christopherson
2022-04-27 2:56 ` Mingwei Zhang
2022-04-27 14:08 ` Sean Christopherson
-- strict thread matches above, loose matches on Subject: below --
2022-03-27 20:35 Mingwei Zhang
2022-03-27 20:43 ` Mingwei Zhang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20220327205803.739336-1-mizhang@google.com \
--to=mizhang@google.com \
--cc=bgardon@google.com \
--cc=dmatlack@google.com \
--cc=jmattson@google.com \
--cc=joro@8bytes.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=vkuznets@redhat.com \
--cc=wanpengli@tencent.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox