Skip to content

Commit

Permalink
KVM: x86/mmu: Ensure TDP MMU roots are freed after yield
Browse files Browse the repository at this point in the history 
Many TDP MMU functions which need to perform some action on all TDP MMU
roots hold a reference on that root so that they can safely drop the MMU
lock in order to yield to other threads. However, when releasing the
reference on the root, there is a bug: the root will not be freed even
if its reference count (root_count) is reduced to 0.

To simplify acquiring and releasing references on TDP MMU root pages, and
to ensure that these roots are properly freed, move the get/put operations
into another TDP MMU root iterator macro.

Moving the get/put operations into an iterator macro also helps
simplify control flow when a root does need to be freed. Note that using
the list_for_each_entry_safe macro would not have been appropriate in
this situation because it could keep a pointer to the next root across
an MMU lock release + reacquire, during which time that root could be
freed.

Reported-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Fixes: faaf05b ("kvm: x86/mmu: Support zapping SPTEs in the TDP MMU")
Fixes: 063afac ("kvm: x86/mmu: Support invalidate range MMU notifier for TDP MMU")
Fixes: a6a0b05 ("kvm: x86/mmu: Support dirty logging for the TDP MMU")
Fixes: 1488199 ("kvm: x86/mmu: Support disabling dirty logging for the tdp MMU")
Signed-off-by: Ben Gardon <bgardon@google.com>
Message-Id: <20210107001935.3732070-1-bgardon@google.com>
Cc: stable@vger.kernel.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
  • Loading branch information
Ben Gardon authored and Paolo Bonzini committed Jan 7, 2021
1 parent 88bf56d commit a889ea5
Showing 1 changed file with 48 additions and 56 deletions.
104 changes: 48 additions & 56 deletions arch/x86/kvm/mmu/tdp_mmu.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,48 @@ void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm)
WARN_ON(!list_empty(&kvm->arch.tdp_mmu_roots));
}

#define for_each_tdp_mmu_root(_kvm, _root) \
static void tdp_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *root)
{
if (kvm_mmu_put_root(kvm, root))
kvm_tdp_mmu_free_root(kvm, root);
}

static inline bool tdp_mmu_next_root_valid(struct kvm *kvm,
struct kvm_mmu_page *root)
{
lockdep_assert_held(&kvm->mmu_lock);

if (list_entry_is_head(root, &kvm->arch.tdp_mmu_roots, link))
return false;

kvm_mmu_get_root(kvm, root);
return true;

}

static inline struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
struct kvm_mmu_page *root)
{
struct kvm_mmu_page *next_root;

next_root = list_next_entry(root, link);
tdp_mmu_put_root(kvm, root);
return next_root;
}

/*
* Note: this iterator gets and puts references to the roots it iterates over.
* This makes it safe to release the MMU lock and yield within the loop, but
* if exiting the loop early, the caller must drop the reference to the most
* recent root. (Unless keeping a live reference is desirable.)
*/
#define for_each_tdp_mmu_root_yield_safe(_kvm, _root) \
for (_root = list_first_entry(&_kvm->arch.tdp_mmu_roots, \
typeof(*_root), link); \
tdp_mmu_next_root_valid(_kvm, _root); \
_root = tdp_mmu_next_root(_kvm, _root))

#define for_each_tdp_mmu_root(_kvm, _root) \
list_for_each_entry(_root, &_kvm->arch.tdp_mmu_roots, link)

bool is_tdp_mmu_root(struct kvm *kvm, hpa_t hpa)
Expand Down Expand Up @@ -447,18 +488,9 @@ bool kvm_tdp_mmu_zap_gfn_range(struct kvm *kvm, gfn_t start, gfn_t end)
struct kvm_mmu_page *root;
bool flush = false;

for_each_tdp_mmu_root(kvm, root) {
/*
* Take a reference on the root so that it cannot be freed if
* this thread releases the MMU lock and yields in this loop.
*/
kvm_mmu_get_root(kvm, root);

for_each_tdp_mmu_root_yield_safe(kvm, root)
flush |= zap_gfn_range(kvm, root, start, end, true);

kvm_mmu_put_root(kvm, root);
}

return flush;
}

Expand Down Expand Up @@ -619,13 +651,7 @@ static int kvm_tdp_mmu_handle_hva_range(struct kvm *kvm, unsigned long start,
int ret = 0;
int as_id;

for_each_tdp_mmu_root(kvm, root) {
/*
* Take a reference on the root so that it cannot be freed if
* this thread releases the MMU lock and yields in this loop.
*/
kvm_mmu_get_root(kvm, root);

for_each_tdp_mmu_root_yield_safe(kvm, root) {
as_id = kvm_mmu_page_as_id(root);
slots = __kvm_memslots(kvm, as_id);
kvm_for_each_memslot(memslot, slots) {
Expand All @@ -647,8 +673,6 @@ static int kvm_tdp_mmu_handle_hva_range(struct kvm *kvm, unsigned long start,
ret |= handler(kvm, memslot, root, gfn_start,
gfn_end, data);
}

kvm_mmu_put_root(kvm, root);
}

return ret;
Expand Down Expand Up @@ -838,21 +862,13 @@ bool kvm_tdp_mmu_wrprot_slot(struct kvm *kvm, struct kvm_memory_slot *slot,
int root_as_id;
bool spte_set = false;

for_each_tdp_mmu_root(kvm, root) {
for_each_tdp_mmu_root_yield_safe(kvm, root) {
root_as_id = kvm_mmu_page_as_id(root);
if (root_as_id != slot->as_id)
continue;

/*
* Take a reference on the root so that it cannot be freed if
* this thread releases the MMU lock and yields in this loop.
*/
kvm_mmu_get_root(kvm, root);

spte_set |= wrprot_gfn_range(kvm, root, slot->base_gfn,
slot->base_gfn + slot->npages, min_level);

kvm_mmu_put_root(kvm, root);
}

return spte_set;
Expand Down Expand Up @@ -906,21 +922,13 @@ bool kvm_tdp_mmu_clear_dirty_slot(struct kvm *kvm, struct kvm_memory_slot *slot)
int root_as_id;
bool spte_set = false;

for_each_tdp_mmu_root(kvm, root) {
for_each_tdp_mmu_root_yield_safe(kvm, root) {
root_as_id = kvm_mmu_page_as_id(root);
if (root_as_id != slot->as_id)
continue;

/*
* Take a reference on the root so that it cannot be freed if
* this thread releases the MMU lock and yields in this loop.
*/
kvm_mmu_get_root(kvm, root);

spte_set |= clear_dirty_gfn_range(kvm, root, slot->base_gfn,
slot->base_gfn + slot->npages);

kvm_mmu_put_root(kvm, root);
}

return spte_set;
Expand Down Expand Up @@ -1029,21 +1037,13 @@ bool kvm_tdp_mmu_slot_set_dirty(struct kvm *kvm, struct kvm_memory_slot *slot)
int root_as_id;
bool spte_set = false;

for_each_tdp_mmu_root(kvm, root) {
for_each_tdp_mmu_root_yield_safe(kvm, root) {
root_as_id = kvm_mmu_page_as_id(root);
if (root_as_id != slot->as_id)
continue;

/*
* Take a reference on the root so that it cannot be freed if
* this thread releases the MMU lock and yields in this loop.
*/
kvm_mmu_get_root(kvm, root);

spte_set |= set_dirty_gfn_range(kvm, root, slot->base_gfn,
slot->base_gfn + slot->npages);

kvm_mmu_put_root(kvm, root);
}
return spte_set;
}
Expand Down Expand Up @@ -1089,21 +1089,13 @@ void kvm_tdp_mmu_zap_collapsible_sptes(struct kvm *kvm,
struct kvm_mmu_page *root;
int root_as_id;

for_each_tdp_mmu_root(kvm, root) {
for_each_tdp_mmu_root_yield_safe(kvm, root) {
root_as_id = kvm_mmu_page_as_id(root);
if (root_as_id != slot->as_id)
continue;

/*
* Take a reference on the root so that it cannot be freed if
* this thread releases the MMU lock and yields in this loop.
*/
kvm_mmu_get_root(kvm, root);

zap_collapsible_spte_range(kvm, root, slot->base_gfn,
slot->base_gfn + slot->npages);

kvm_mmu_put_root(kvm, root);
}
}

Expand Down

0 comments on commit a889ea5

Please sign in to comment.