Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <bf99bb5c-f3e1-670d-cdd4-261215160524@redhat.com>
Date:   Wed, 1 Dec 2021 21:49:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.0
Subject: Re: [PATCH 04/28] KVM: x86/mmu: Retry page fault if root is
 invalidated by memslot update
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>
Cc:     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,
        Hou Wenlong <houwenlong93@linux.alibaba.com>,
        Ben Gardon <bgardon@google.com>
References: <20211120045046.3940942-1-seanjc@google.com>
 <20211120045046.3940942-5-seanjc@google.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <20211120045046.3940942-5-seanjc@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/20/21 05:50, Sean Christopherson wrote:
> Bail from the page fault handler if the root shadow page was obsoleted by
> a memslot update.  Do the check _after_ acuiring mmu_lock, as the TDP MMU
> doesn't rely on the memslot/MMU generation, and instead relies on the
> root being explicit marked invalid by kvm_mmu_zap_all_fast(), which takes
> mmu_lock for write.
> 
> For the TDP MMU, inserting a SPTE into an obsolete root can leak a SP if
> kvm_tdp_mmu_zap_invalidated_roots() has already zapped the SP, i.e. has
> moved past the gfn associated with the SP.
> 
> For other MMUs, the resulting behavior is far more convoluted, though
> unlikely to be truly problematic.  Installing SPs/SPTEs into the obsolete
> root isn't directly problematic, as the obsolete root will be unloaded
> and dropped before the vCPU re-enters the guest.  But because the legacy
> MMU tracks shadow pages by their role, any SP created by the fault can
> can be reused in the new post-reload root.  Again, that _shouldn't_ be
> problematic as any leaf child SPTEs will be created for the current/valid
> memslot generation, and kvm_mmu_get_page() will not reuse child SPs from
> the old generation as they will be flagged as obsolete.  But, given that
> continuing with the fault is pointess (the root will be unloaded), apply
> the check to all MMUs.
> 
> Fixes: b7cccd397f31 ("KVM: x86/mmu: Fast invalidation for TDP MMU")
> Cc: stable@vger.kernel.org
> Cc: Ben Gardon <bgardon@google.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/mmu/mmu.c         | 23 +++++++++++++++++++++--
>   arch/x86/kvm/mmu/paging_tmpl.h |  3 ++-
>   2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index d2ad12a4d66e..31ce913efe37 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -1936,7 +1936,11 @@ static void mmu_audit_disable(void) { }
>   
>   static bool is_obsolete_sp(struct kvm *kvm, struct kvm_mmu_page *sp)
>   {
> -	return sp->role.invalid ||
> +	if (sp->role.invalid)
> +		return true;
> +
> +	/* TDP MMU pages due not use the MMU generation. */
> +	return !sp->tdp_mmu_page &&
>   	       unlikely(sp->mmu_valid_gen != kvm->arch.mmu_valid_gen);
>   }
>   
> @@ -3976,6 +3980,20 @@ static bool kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
>   	return true;
>   }
>   
> +/*
> + * Returns true if the page fault is stale and needs to be retried, i.e. if the
> + * root was invalidated by a memslot update or a relevant mmu_notifier fired.
> + */
> +static bool is_page_fault_stale(struct kvm_vcpu *vcpu,
> +				struct kvm_page_fault *fault, int mmu_seq)
> +{
> +	if (is_obsolete_sp(vcpu->kvm, to_shadow_page(vcpu->arch.mmu->root_hpa)))
> +		return true;
> +
> +	return fault->slot &&
> +	       mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, fault->hva);
> +}
> +
>   static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
>   {
>   	bool is_tdp_mmu_fault = is_tdp_mmu(vcpu->arch.mmu);
> @@ -4013,8 +4031,9 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>   	else
>   		write_lock(&vcpu->kvm->mmu_lock);
>   
> -	if (fault->slot && mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, fault->hva))
> +	if (is_page_fault_stale(vcpu, fault, mmu_seq))
>   		goto out_unlock;
> +
>   	r = make_mmu_pages_available(vcpu);
>   	if (r)
>   		goto out_unlock;
> diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
> index f87d36898c44..708a5d297fe1 100644
> --- a/arch/x86/kvm/mmu/paging_tmpl.h
> +++ b/arch/x86/kvm/mmu/paging_tmpl.h
> @@ -911,7 +911,8 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>   
>   	r = RET_PF_RETRY;
>   	write_lock(&vcpu->kvm->mmu_lock);
> -	if (fault->slot && mmu_notifier_retry_hva(vcpu->kvm, mmu_seq, fault->hva))
> +
> +	if (is_page_fault_stale(vcpu, fault, mmu_seq))
>   		goto out_unlock;
>   
>   	kvm_mmu_audit(vcpu, AUDIT_PRE_PAGE_FAULT);
> 

Queued, thanks.

Paolo