linux-kernel - Re: [PATCH v2 21/28] KVM: x86/mmu: Flush TLBs after zap in TDP MMU PF handler

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YB3i1UXM/uIzHJuD@google.com>
Date:   Fri, 5 Feb 2021 16:29:09 -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 Xu <peterx@...hat.com>, Peter Shier <pshier@...gle.com>,
        Peter Feiner <pfeiner@...gle.com>,
        Junaid Shahid <junaids@...gle.com>,
        Jim Mattson <jmattson@...gle.com>,
        Yulei Zhang <yulei.kernel@...il.com>,
        Wanpeng Li <kernellwp@...il.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Xiao Guangrong <xiaoguangrong.eric@...il.com>
Subject: Re: [PATCH v2 21/28] KVM: x86/mmu: Flush TLBs after zap in TDP MMU
 PF handler

On Tue, Feb 02, 2021, Ben Gardon wrote:
> +static inline bool tdp_mmu_zap_spte_atomic(struct kvm *kvm,
> +					   struct tdp_iter *iter)
> +{
> +	/*
> +	 * Freeze the SPTE by setting it to a special,
> +	 * non-present value. This will stop other threads from
> +	 * immediately installing a present entry in its place
> +	 * before the TLBs are flushed.
> +	 */
> +	if (!tdp_mmu_set_spte_atomic(kvm, iter, REMOVED_SPTE))
> +		return false;
> +
> +	kvm_flush_remote_tlbs_with_address(kvm, iter->gfn,
> +					   KVM_PAGES_PER_HPAGE(iter->level));
> +
> +	/*
> +	 * No other thread can overwrite the removed SPTE as they
> +	 * must either wait on the MMU lock or use
> +	 * tdp_mmu_set_spte_atomic which will not overrite the
> +	 * special removed SPTE value. No bookkeeping is needed
> +	 * here since the SPTE is going from non-present
> +	 * to non-present.
> +	 */

Can we expand these comments out to 80 chars before the final/official push?

> +	WRITE_ONCE(*iter->sptep, 0);
> +
> +	return true;
> +}
> +
>  
>  /*
>   * __tdp_mmu_set_spte - Set a TDP MMU SPTE and handle the associated bookkeeping
> @@ -523,6 +562,15 @@ static inline void __tdp_mmu_set_spte(struct kvm *kvm, struct tdp_iter *iter,
>  
>  	lockdep_assert_held_write(&kvm->mmu_lock);
>  
> +	/*
> +	 * No thread should be using this function to set SPTEs to the
> +	 * temporary removed SPTE value.
> +	 * If operating under the MMU lock in read mode, tdp_mmu_set_spte_atomic
> +	 * should be used. If operating under the MMU lock in write mode, the
> +	 * use of the removed SPTE should not be necessary.
> +	 */
> +	WARN_ON(iter->old_spte == REMOVED_SPTE);
> +
>  	WRITE_ONCE(*rcu_dereference(iter->sptep), new_spte);
>  
>  	__handle_changed_spte(kvm, as_id, iter->gfn, iter->old_spte, new_spte,
> @@ -790,12 +838,9 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, gpa_t gpa, u32 error_code,
>  		 */
>  		if (is_shadow_present_pte(iter.old_spte) &&
>  		    is_large_pte(iter.old_spte)) {
> -			if (!tdp_mmu_set_spte_atomic(vcpu->kvm, &iter, 0))
> +			if (!tdp_mmu_zap_spte_atomic(vcpu->kvm, &iter))
>  				break;
>  
> -			kvm_flush_remote_tlbs_with_address(vcpu->kvm, iter.gfn,
> -					KVM_PAGES_PER_HPAGE(iter.level));
> -
>  			/*
>  			 * The iter must explicitly re-read the spte here
>  			 * because the new value informs the !present
> -- 
> 2.30.0.365.g02bc693789-goog
>