Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
Message-ID: <c94b3aec-981e-8557-ba29-0094b075b8e4@redhat.com>
Date:   Fri, 10 Dec 2021 13:41:56 +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 1/7] KVM: x86: Retry page fault if MMU reload is pending
 and root has no sp
Content-Language: en-US
To:     Sean Christopherson <seanjc@google.com>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>
Cc:     David Hildenbrand <david@redhat.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        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, Maxim Levitsky <mlevitsk@redhat.com>,
        Ben Gardon <bgardon@google.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>
References: <20211209060552.2956723-1-seanjc@google.com>
 <20211209060552.2956723-2-seanjc@google.com>
From:   Paolo Bonzini <pbonzini@redhat.com>
In-Reply-To: <20211209060552.2956723-2-seanjc@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/9/21 07:05, Sean Christopherson wrote:
> +	/* Special roots, e.g. pae_root, are not backed by shadow pages. */
> +	if (sp && is_obsolete_sp(vcpu->kvm, sp))
> +		return true;
> +
> +	/*
> +	 * Roots without an associated shadow page are considered invalid if
> +	 * there is a pending request to free obsolete roots.  The request is
> +	 * only a hint that the current root_may_  be obsolete and needs to be
> +	 * reloaded, e.g. if the guest frees a PGD that KVM is tracking as a
> +	 * previous root, then __kvm_mmu_prepare_zap_page() signals all vCPUs
> +	 * to reload even if no vCPU is actively using the root.
> +	 */
> +	if (!sp && kvm_test_request(KVM_REQ_MMU_RELOAD, vcpu))
>   		return true;

Hmm I don't understand this (or maybe I do and I just don't like what I
understand).

KVM_REQ_MMU_RELOAD is raised after kvm->arch.mmu_valid_gen is fixed (of
course, otherwise the other CPU might just not see any obsoleted page
from the legacy MMU), therefore any check on KVM_REQ_MMU_RELOAD is just
advisory.

This is not a problem per se; in the other commit message you said,

     For other MMUs, the resulting behavior is far more convoluted,
     though unlikely to be truly problematic.

but it's unnecessarily complicating the logic.  I'm more inclined to
just play it simple and make the special roots process the page fault;
Jiangshan's work should clean things up a bit:

--------- 8< -------------
 From 0c1e30d4e7e17692668d960452107f983dd2c9a9 Mon Sep 17 00:00:00 2001
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Fri, 10 Dec 2021 07:41:02 -0500
Subject: [PATCH] KVM: x86: Do not check obsoleteness of roots that have no sp
  attached

The "special" roots, e.g. pae_root when KVM uses PAE paging, are not
backed by a shadow page.  Running with TDP disabled or with nested NPT
explodes spectaculary due to dereferencing a NULL shadow page pointer.
Play nice with a NULL shadow page when checking for an obsolete root in
is_page_fault_stale().

Fixes: a955cad84cda ("KVM: x86/mmu: Retry page fault if root is invalidated by memslot update")
Cc: stable@vger.kernel.org
Cc: Maxim Levitsky <mlevitsk@redhat.com>
Analyzed-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index e2e1d012df22..4a3bcdd3cfe7 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -3987,7 +3987,17 @@ static bool kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault,
  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)))
+	struct kvm_mmu_page *sp = to_shadow_page(vcpu->arch.mmu->root_hpa);
+
+	/*
+	 * Special roots, e.g. pae_root, are not backed by shadow pages
+	 * so there isn't an easy way to detect if they're obsolete.
+	 * If they are, any child SPTE created by the fault will be useless
+	 * (they will instantly be treated as obsolete because they don't
+	 * match the mmu_valid_gen); but they will not leak, so just play
+	 * it simple.
+	 */
+	if (sp && is_obsolete_sp(vcpu->kvm, sp))
  		return true;
  
  	return fault->slot &&