| 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: <YEuVVpySnR4Fg6bh@google.com>
Date: Fri, 12 Mar 2021 08:22:46 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Ben Gardon <bgardon@...gle.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
Paolo Bonzini <pbonzini@...hat.com>,
Peter Shier <pshier@...gle.com>,
Jim Mattson <jmattson@...gle.com>
Subject: Re: [PATCH 2/4] KVM: x86/mmu: Fix RCU usage for tdp_iter_root_pt
On Thu, Mar 11, 2021, Ben Gardon wrote:
> The root page table in the TDP MMU paging structure is not protected
> with RCU, but rather by the root_count in the associated SP. As a result
> it is safe for tdp_iter_root_pt to simply return a u64 *. This sidesteps
> the complexities assoicated with propagating the __rcu annotation
> around.
>
> Reported-by: kernel test robot <lkp@...xxxxxx>
> Signed-off-by: Ben Gardon <bgardon@...gle.com>
> ---
> arch/x86/kvm/mmu/tdp_iter.c | 10 ++++++++--
> arch/x86/kvm/mmu/tdp_iter.h | 2 +-
> arch/x86/kvm/mmu/tdp_mmu.c | 4 ++--
> 3 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c
> index e5f148106e20..8e2c053533b6 100644
> --- a/arch/x86/kvm/mmu/tdp_iter.c
> +++ b/arch/x86/kvm/mmu/tdp_iter.c
> @@ -159,8 +159,14 @@ void tdp_iter_next(struct tdp_iter *iter)
> iter->valid = false;
> }
>
> -tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter)
> +u64 *tdp_iter_root_pt(struct tdp_iter *iter)
> {
> - return iter->pt_path[iter->root_level - 1];
> + /*
> + * Though it is stored in an array of tdp_ptep_t for convenience,
> + * the root PT is not actually protected by RCU, but by the root
> + * count on the associated struct kvm_mmu_page. As a result it's
> + * safe to rcu_dereference and return the value here.
I'm not a big fan of this comment. It implies that calling tdp_iter_root_pt()
without RCU protection is completely ok, but that's not true, as rcu_dereferecne()
will complain when CONFIG_PROVE_RCU=1.
There's also a good opportunity to streamline the the helper here, since both
callers use the root only to get to the associated shadow page, and that's only
done to get the as_id. If we provide tdp_iter_as_id() then the need for a
comment goes away and we shave a few lines of code.
That being said, an even better option would be to store as_id in the TDP iter.
The cost on the stack is negligible, and while the early sptep->as_id lookup
will be unnecessary in some cases, it will be a net win when setting multiple
sptes, e.g. in mmu_notifier callbacks.
Compile tested only...
>From 02fb9cd2aa52d0afd318e93661d0212ccdb54218 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@...gle.com>
Date: Fri, 12 Mar 2021 08:12:21 -0800
Subject: [PATCH] KVM: x86/mmu: Store the address space ID in the TDP iterator
Store the address space ID in the TDP iterator so that it can be
retrieved without having to bounce through the root shadow page. This
streamlines the code and fixes a Sparse warning about not properly using
rcu_dereference() when grabbing the ID from the root on the fly.
Reported-by: kernel test robot <lkp@...el.com>
Cc: Ben Gardon <bgardon@...gle.com>
Signed-off-by: Sean Christopherson <seanjc@...gle.com>
---
arch/x86/kvm/mmu/mmu_internal.h | 5 +++++
arch/x86/kvm/mmu/tdp_iter.c | 7 +------
arch/x86/kvm/mmu/tdp_iter.h | 3 ++-
arch/x86/kvm/mmu/tdp_mmu.c | 23 +++++------------------
4 files changed, 13 insertions(+), 25 deletions(-)
diff --git a/arch/x86/kvm/mmu/mmu_internal.h b/arch/x86/kvm/mmu/mmu_internal.h
index ec4fc28b325a..e844078d2374 100644
--- a/arch/x86/kvm/mmu/mmu_internal.h
+++ b/arch/x86/kvm/mmu/mmu_internal.h
@@ -119,6 +119,11 @@ static inline bool kvm_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *sp)
return !sp->root_count;
}
+static inline int kvm_mmu_page_as_id(struct kvm_mmu_page *sp)
+{
+ return sp->role.smm ? 1 : 0;
+}
+
/*
* Return values of handle_mmio_page_fault, mmu.page_fault, and fast_page_fault().
*
diff --git a/arch/x86/kvm/mmu/tdp_iter.c b/arch/x86/kvm/mmu/tdp_iter.c
index e5f148106e20..55d0ce2185a5 100644
--- a/arch/x86/kvm/mmu/tdp_iter.c
+++ b/arch/x86/kvm/mmu/tdp_iter.c
@@ -40,6 +40,7 @@ void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level,
iter->gfn = round_gfn_for_level(iter->next_last_level_gfn, iter->level);
tdp_iter_refresh_sptep(iter);
+ iter->as_id = kvm_mmu_page_as_id(sptep_to_sp(root_pt));
iter->valid = true;
}
@@ -158,9 +159,3 @@ void tdp_iter_next(struct tdp_iter *iter)
} while (try_step_up(iter));
iter->valid = false;
}
-
-tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter)
-{
- return iter->pt_path[iter->root_level - 1];
-}
-
diff --git a/arch/x86/kvm/mmu/tdp_iter.h b/arch/x86/kvm/mmu/tdp_iter.h
index 4cc177d75c4a..df9c84713f5b 100644
--- a/arch/x86/kvm/mmu/tdp_iter.h
+++ b/arch/x86/kvm/mmu/tdp_iter.h
@@ -36,6 +36,8 @@ struct tdp_iter {
int min_level;
/* The iterator's current level within the paging structure */
int level;
+ /* The address space ID, i.e. SMM vs. regular. */
+ int as_id;
/* A snapshot of the value at sptep */
u64 old_spte;
/*
@@ -62,6 +64,5 @@ tdp_ptep_t spte_to_child_pt(u64 pte, int level);
void tdp_iter_start(struct tdp_iter *iter, u64 *root_pt, int root_level,
int min_level, gfn_t next_last_level_gfn);
void tdp_iter_next(struct tdp_iter *iter);
-tdp_ptep_t tdp_iter_root_pt(struct tdp_iter *iter);
#endif /* __KVM_X86_MMU_TDP_ITER_H */
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 21cbbef0ee57..9f436aa14663 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -190,11 +190,6 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
u64 old_spte, u64 new_spte, int level,
bool shared);
-static int kvm_mmu_page_as_id(struct kvm_mmu_page *sp)
-{
- return sp->role.smm ? 1 : 0;
-}
-
static void handle_changed_spte_acc_track(u64 old_spte, u64 new_spte, int level)
{
if (!is_shadow_present_pte(old_spte) || !is_last_spte(old_spte, level))
@@ -472,10 +467,6 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
struct tdp_iter *iter,
u64 new_spte)
{
- u64 *root_pt = tdp_iter_root_pt(iter);
- struct kvm_mmu_page *root = sptep_to_sp(root_pt);
- int as_id = kvm_mmu_page_as_id(root);
-
lockdep_assert_held_read(&kvm->mmu_lock);
/*
@@ -489,8 +480,8 @@ static inline bool tdp_mmu_set_spte_atomic(struct kvm *kvm,
new_spte) != iter->old_spte)
return false;
- handle_changed_spte(kvm, as_id, iter->gfn, iter->old_spte, new_spte,
- iter->level, true);
+ handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte,
+ new_spte, iter->level, true);
return true;
}
@@ -544,10 +535,6 @@ static inline void __tdp_mmu_set_spte(struct kvm *kvm, struct tdp_iter *iter,
u64 new_spte, bool record_acc_track,
bool record_dirty_log)
{
- tdp_ptep_t root_pt = tdp_iter_root_pt(iter);
- struct kvm_mmu_page *root = sptep_to_sp(root_pt);
- int as_id = kvm_mmu_page_as_id(root);
-
lockdep_assert_held_write(&kvm->mmu_lock);
/*
@@ -561,13 +548,13 @@ static inline void __tdp_mmu_set_spte(struct kvm *kvm, struct tdp_iter *iter,
WRITE_ONCE(*rcu_dereference(iter->sptep), new_spte);
- __handle_changed_spte(kvm, as_id, iter->gfn, iter->old_spte, new_spte,
- iter->level, false);
+ __handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte,
+ new_spte, iter->level, false);
if (record_acc_track)
handle_changed_spte_acc_track(iter->old_spte, new_spte,
iter->level);
if (record_dirty_log)
- handle_changed_spte_dirty_log(kvm, as_id, iter->gfn,
+ handle_changed_spte_dirty_log(kvm, iter->as_id, iter->gfn,
iter->old_spte, new_spte,
iter->level);
}
--
Powered by blists - more mailing lists