| 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
| ||
|
Date: Thu, 16 Feb 2023 18:39:26 +0200
From: Zhi Wang <zhi.wang.linux@...il.com>
To: isaku.yamahata@...el.com
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
isaku.yamahata@...il.com, Paolo Bonzini <pbonzini@...hat.com>,
erdemaktas@...gle.com, Sean Christopherson <seanjc@...gle.com>,
Sagi Shahar <sagis@...gle.com>,
David Matlack <dmatlack@...gle.com>
Subject: Re: [PATCH v11 044/113] KVM: x86/tdp_mmu: Make
handle_changed_spte() return value
On Thu, 12 Jan 2023 08:31:52 -0800
isaku.yamahata@...el.com wrote:
Some typos below:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
>
> TDX operation can fail with TDX_OPERAND_BUSY when multiple vcpu try to
vcpus
> operation on same TDX resource like Secure EPT. It doesn't spin and returns
operate
> busy error to VMM so that VMM has to take action, e.g. retry or whatever.
>
> Because TDP MMU uses read spin lock for scalability, spinlock around seam
> call busts TDP MMU effort. The other option is to let SEAMCALL fail and
> page fault handler should retry. Make handle_changed_spte() and its caller
> return values so that kvm page fault handler can return on such cases. This
> patch makes it return only zero.
>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> ---
> arch/x86/kvm/mmu/tdp_mmu.c | 72 +++++++++++++++++++++++++-------------
> 1 file changed, 47 insertions(+), 25 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 7ab498b80214..4fb07f91e5d6 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -349,9 +349,9 @@ hpa_t kvm_tdp_mmu_get_vcpu_root_hpa(struct kvm_vcpu *vcpu)
> return __pa(root->spt);
> }
>
> -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 __must_check handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> + u64 old_spte, u64 new_spte, int level,
> + bool shared);
>
> static void handle_changed_spte_acc_track(u64 old_spte, u64 new_spte, int level)
> {
> @@ -445,6 +445,7 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
> struct kvm_mmu_page *sp = sptep_to_sp(rcu_dereference(pt));
> int level = sp->role.level;
> gfn_t base_gfn = sp->gfn;
> + int ret;
> int i;
>
> trace_kvm_mmu_prepare_zap_page(sp);
> @@ -516,8 +517,14 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
> old_spte = kvm_tdp_mmu_write_spte(sptep, old_spte,
> REMOVED_SPTE, level);
> }
> - handle_changed_spte(kvm, kvm_mmu_page_as_id(sp), gfn,
> - old_spte, REMOVED_SPTE, level, shared);
> + ret = handle_changed_spte(kvm, kvm_mmu_page_as_id(sp), gfn,
> + old_spte, REMOVED_SPTE, level, shared);
> + /*
> + * We are removing page tables. Because in TDX case we don't
> + * zap private page tables except tearing down VM. It means
> + * no race condition.
> + */
> + WARN_ON_ONCE(ret);
> }
>
> call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
> @@ -538,9 +545,9 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
> * Handle bookkeeping that might result from the modification of a SPTE.
> * This function must be called for all TDP SPTE modifications.
> */
> -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 __must_check __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> + u64 old_spte, u64 new_spte, int level,
> + bool shared)
> {
> bool was_present = is_shadow_present_pte(old_spte);
> bool is_present = is_shadow_present_pte(new_spte);
> @@ -576,7 +583,7 @@ static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> }
>
> if (old_spte == new_spte)
> - return;
> + return 0;
>
> trace_kvm_tdp_mmu_spte_changed(as_id, gfn, level, old_spte, new_spte);
>
> @@ -605,7 +612,7 @@ static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> "a temporary removed SPTE.\n"
> "as_id: %d gfn: %llx old_spte: %llx new_spte: %llx level: %d",
> as_id, gfn, old_spte, new_spte, level);
> - return;
> + return 0;
> }
>
> if (is_leaf != was_leaf)
> @@ -624,17 +631,25 @@ static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> if (was_present && !was_leaf &&
> (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed)))
> handle_removed_pt(kvm, spte_to_child_pt(old_spte, level), shared);
> +
> + return 0;
> }
>
> -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 __must_check handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> + u64 old_spte, u64 new_spte, int level,
> + bool shared)
> {
> - __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level,
> - shared);
> + int ret;
> +
> + ret = __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level,
> + shared);
> + if (ret)
> + return ret;
> +
> handle_changed_spte_acc_track(old_spte, new_spte, level);
> handle_changed_spte_dirty_log(kvm, as_id, gfn, old_spte,
> new_spte, level);
> + return 0;
> }
>
> /*
> @@ -653,12 +668,14 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> * * -EBUSY - If the SPTE cannot be set. In this case this function will have
> * no side-effects other than setting iter->old_spte to the last
> * known value of the spte.
> + * * -EAGAIN - Same to -EBUSY. But the source is from callbacks for private spt
> */
> -static inline int tdp_mmu_set_spte_atomic(struct kvm *kvm,
> - struct tdp_iter *iter,
> - u64 new_spte)
> +static inline int __must_check tdp_mmu_set_spte_atomic(struct kvm *kvm,
> + struct tdp_iter *iter,
> + u64 new_spte)
> {
> u64 *sptep = rcu_dereference(iter->sptep);
> + int ret;
>
> /*
> * The caller is responsible for ensuring the old SPTE is not a REMOVED
> @@ -677,15 +694,16 @@ static inline int tdp_mmu_set_spte_atomic(struct kvm *kvm,
> if (!try_cmpxchg64(sptep, &iter->old_spte, new_spte))
> return -EBUSY;
>
> - __handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte,
> - new_spte, iter->level, true);
> - handle_changed_spte_acc_track(iter->old_spte, new_spte, iter->level);
> + ret = __handle_changed_spte(kvm, iter->as_id, iter->gfn, iter->old_spte,
> + new_spte, iter->level, true);
> + if (!ret)
> + handle_changed_spte_acc_track(iter->old_spte, new_spte, iter->level);
>
> - return 0;
> + return ret;
> }
>
----
> -static inline int tdp_mmu_zap_spte_atomic(struct kvm *kvm,
> - struct tdp_iter *iter)
> +static inline int __must_check tdp_mmu_zap_spte_atomic(struct kvm *kvm,
> + struct tdp_iter *iter)
> {
> int ret;
>
----
The above part doesn't belong to this patch.
> @@ -750,6 +768,8 @@ static u64 __tdp_mmu_set_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
> u64 old_spte, u64 new_spte, gfn_t gfn, int level,
> bool record_acc_track, bool record_dirty_log)
> {
> + int ret;
> +
> lockdep_assert_held_write(&kvm->mmu_lock);
>
> /*
> @@ -763,7 +783,9 @@ static u64 __tdp_mmu_set_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
>
> old_spte = kvm_tdp_mmu_write_spte(sptep, old_spte, new_spte, level);
>
> - __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level, false);
> + ret = __handle_changed_spte(kvm, as_id, gfn, old_spte, new_spte, level, false);
> + /* Because write spin lock is held, no race. It should success. */
> + WARN_ON_ONCE(ret);
>
> if (record_acc_track)
> handle_changed_spte_acc_track(old_spte, new_spte, level);
Powered by blists - more mailing lists