Re: [PATCH] KVM: x86: Check for invalid/obsolete root *after* making MMU pages available

On Sat, Aug 08, 2026, Sergey Senozhatsky wrote:
> From: Sean Christopherson <[email protected]>
> 
> Check for a "stale" page fault, i.e. for an invalid and/or obsolete root,
> after making MMU pages available for the shadow MMU.  If reclaiming shadow
> pages zaps an in-use root, i.e. marks it invalid, then KVM will attempt to
> map memory into an invalid root.  On its own, populating an invalid root is
> "fine", but because child shadow pages inherit their parent's role, any
> children created during the map/fetch will be created as invalid pages,
> thus violating KVM's invariant that invalid pages are never on the list of
> active MMU pages.
> 
> Note, the underlying flaw has existed since KVM first started tracking
> invalid roots in 2008 (commit 2e53d63acba7, "KVM: MMU: ignore zapped root
> pagetables"), but the true badness only came along in 2020 (Linux 5.9)
> with the invariant that invalid shadow pages can't be on the list of
> active pages.
> 
> Note #2, inheriting role.invalid when creating child shadow pages is also
> far from ideal; that flaw will be addressed separately.
> 
> Conflicts:
> 	arch/x86/kvm/mmu/mmu.c
> 	arch/x86/kvm/mmu/paging_tmpl.h
> 
> Reported-by: Hyunwoo Kim <[email protected]>
> Fixes: f95eec9bed76 ("KVM: x86/mmu: Don't put invalid SPs back on the list of active pages")
> Cc: [email protected]
> Signed-off-by: Sean Christopherson <[email protected]>
> Signed-off-by: Paolo Bonzini <[email protected]>
> (cherry picked from commit 2abd5287f08319fa35764566b15c6e22cb1068db)
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> ---
>  arch/x86/kvm/mmu/mmu.c         | 11 ++++++-----
>  arch/x86/kvm/mmu/paging_tmpl.h | 10 ++++++----
>  2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 7785da8f0ad3..1f4d306e5c81 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4272,22 +4272,23 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
>  
>  	orig_pfn = fault->pfn;
>  
> -	r = RET_PF_RETRY;
> -
>  	if (is_tdp_mmu_fault)
>  		read_lock(&vcpu->kvm->mmu_lock);
>  	else
>  		write_lock(&vcpu->kvm->mmu_lock);
>  
> -	if (is_page_fault_stale(vcpu, fault, mmu_seq))
> -		goto out_unlock;
> -
>  	if (is_tdp_mmu_fault) {
>  		r = kvm_tdp_mmu_map(vcpu, fault);

is_page_fault_stale() needs be checked for the TDP MMU as well.  I had the
pleasure of doing this cherry-pick for an internal kernel, this is what I thought
yielded the easiest-to-read sequence:

        if (is_tdp_mmu_fault) {
                read_lock(&vcpu->kvm->mmu_lock);
        } else {
                write_lock(&vcpu->kvm->mmu_lock);

                r = make_mmu_pages_available(vcpu);
                if (r)
                        goto out_unlock;
        }

        if (is_page_fault_stale(vcpu, fault, mmu_seq)) {
                r = RET_PF_RETRY;
                goto out_unlock;
        }

        if (is_tdp_mmu_fault)
                r = kvm_tdp_mmu_map(vcpu, fault);
        else
                r = __direct_map(vcpu, fault);

As a diff...

diff --git arch/x86/kvm/mmu/mmu.c arch/x86/kvm/mmu/mmu.c
index 7785da8f0ad3..d05bd467dc5d 100644
--- arch/x86/kvm/mmu/mmu.c
+++ arch/x86/kvm/mmu/mmu.c
@@ -4272,24 +4272,25 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 
        orig_pfn = fault->pfn;
 
-       r = RET_PF_RETRY;
-
-       if (is_tdp_mmu_fault)
-               read_lock(&vcpu->kvm->mmu_lock);
-       else
-               write_lock(&vcpu->kvm->mmu_lock);
-
-       if (is_page_fault_stale(vcpu, fault, mmu_seq))
-               goto out_unlock;
-
        if (is_tdp_mmu_fault) {
-               r = kvm_tdp_mmu_map(vcpu, fault);
+               read_lock(&vcpu->kvm->mmu_lock);
        } else {
+               write_lock(&vcpu->kvm->mmu_lock);
+
                r = make_mmu_pages_available(vcpu);
                if (r)
                        goto out_unlock;
+       }
+
+       if (is_page_fault_stale(vcpu, fault, mmu_seq)) {
+               r = RET_PF_RETRY;
+               goto out_unlock;
+       }
+
+       if (is_tdp_mmu_fault)
+               r = kvm_tdp_mmu_map(vcpu, fault);
+       else
                r = __direct_map(vcpu, fault);
-       }
 
 out_unlock:
        if (is_tdp_mmu_fault)
diff --git arch/x86/kvm/mmu/paging_tmpl.h arch/x86/kvm/mmu/paging_tmpl.h
index 1c7d73b8081c..75fd2b7dc0e5 100644
--- arch/x86/kvm/mmu/paging_tmpl.h
+++ arch/x86/kvm/mmu/paging_tmpl.h
@@ -869,15 +869,17 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 
        orig_pfn = fault->pfn;
 
-       r = RET_PF_RETRY;
        write_lock(&vcpu->kvm->mmu_lock);
 
-       if (is_page_fault_stale(vcpu, fault, mmu_seq))
-               goto out_unlock;
-
        r = make_mmu_pages_available(vcpu);
        if (r)
                goto out_unlock;
+
+       if (is_page_fault_stale(vcpu, fault, mmu_seq)) {
+               r = RET_PF_RETRY;
+               goto out_unlock;
+       }
+
        r = FNAME(fetch)(vcpu, fault, &walker);
 
 out_unlock: