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

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

[Paolo Bonzini]

From: Sean Christopherson <seanjc@google.com>

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.

Reported-by: Hyunwoo Kim <imv4bel@gmail.com>
Fixes: f95eec9bed76 ("KVM: x86/mmu: Don't put invalid SPs back on the list of active pages")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/mmu/mmu.c         |  9 +++++----
 arch/x86/kvm/mmu/paging_tmpl.h | 10 ++++++----
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 234d0a95abf5..41f92ed1ca37 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4852,16 +4852,17 @@ static int direct_page_fault(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 	if (r != RET_PF_CONTINUE)
 		return r;
 
-	r = RET_PF_RETRY;
 	write_lock(&vcpu->kvm->mmu_lock);
 
-	if (is_page_fault_stale(vcpu, fault))
-		goto out_unlock;
-
 	r = make_mmu_pages_available(vcpu);
 	if (r)
 		goto out_unlock;
 
+	if (is_page_fault_stale(vcpu, fault)) {
+		r = RET_PF_RETRY;
+		goto out_unlock;
+	}
+
 	r = direct_map(vcpu, fault);
 
 out_unlock:
diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index df3ae0c7ec2c..1ba840a73b7a 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -864,15 +864,17 @@ static int FNAME(page_fault)(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 	}
 #endif
 
-	r = RET_PF_RETRY;
 	write_lock(&vcpu->kvm->mmu_lock);
 
-	if (is_page_fault_stale(vcpu, fault))
-		goto out_unlock;
-
 	r = make_mmu_pages_available(vcpu);
 	if (r)
 		goto out_unlock;
+
+	if (is_page_fault_stale(vcpu, fault)) {
+		r = RET_PF_RETRY;
+		goto out_unlock;
+	}
+
 	r = FNAME(fetch)(vcpu, fault, &walker);
 
 out_unlock:
-- 
2.55.0

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

[Sergey Senozhatsky]

From: Sean Christopherson <seanjc@google.com>

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 <imv4bel@gmail.com>
Fixes: f95eec9bed76 ("KVM: x86/mmu: Don't put invalid SPs back on the list of active pages")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
(cherry picked from commit 2abd5287f08319fa35764566b15c6e22cb1068db)
Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 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);
 	} else {
 		r = make_mmu_pages_available(vcpu);
 		if (r)
 			goto out_unlock;
+
+		if (is_page_fault_stale(vcpu, fault)) {
+			r = RET_PF_RETRY;
+			goto out_unlock;
+		}
+
 		r = __direct_map(vcpu, fault);
 	}
 
diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index 1c7d73b8081c..a048305e08dd 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/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)) {
+		r = RET_PF_RETRY;
+		goto out_unlock;
+	}
+
 	r = FNAME(fetch)(vcpu, fault, &walker);
 
 out_unlock:
-- 
2.55.0.654.g21b8a5bc05-goog

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

[Sergey Senozhatsky]

On (26/08/08 15:22), Sergey Senozhatsky wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> 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

Sean, can you please take a look if conflict resolution was done
correctly?

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

[Sean Christopherson]

On Sat, Aug 08, 2026, Sergey Senozhatsky wrote:
> On (26/08/08 15:22), Sergey Senozhatsky wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > 
> > 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
> 
> Sean, can you please take a look if conflict resolution was done
> correctly?

What kernel is this for?

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

[Sergey Senozhatsky]

On (26/08/10 07:08), Sean Christopherson wrote:
> On Sat, Aug 08, 2026, Sergey Senozhatsky wrote:
> > On (26/08/08 15:22), Sergey Senozhatsky wrote:
> > > From: Sean Christopherson <seanjc@google.com>
> > > 
> > > 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
> > 
> > Sean, can you please take a look if conflict resolution was done
> > correctly?
> 
> What kernel is this for?

Oh... good point.  I forgot to specify [PATCH 6.1.y]

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

[Sean Christopherson]

On Sat, Aug 08, 2026, Sergey Senozhatsky wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> 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 <imv4bel@gmail.com>
> Fixes: f95eec9bed76 ("KVM: x86/mmu: Don't put invalid SPs back on the list of active pages")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> (cherry picked from commit 2abd5287f08319fa35764566b15c6e22cb1068db)
> Signed-off-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  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:

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

[Sasha Levin]

> Conflicts:
> 	arch/x86/kvm/mmu/mmu.c
> 	arch/x86/kvm/mmu/paging_tmpl.h

The 6.1 resolution has two problems I think.

The new call sites pass two arguments, but 6.1 declares
is_page_fault_stale(vcpu, fault, mmu_seq) - so I'm not sure why you're not
seeing a build breakage.

Upstream could move the check because direct_page_fault() is shadow-MMU-only
there. On 6.1 it still serves both MMUs and dispatches on is_tdp_mmu_fault, so
relocating the check into the shadow arm leaves kvm_tdp_mmu_map() with no check
and no mmu_invalidate_retry_hva() retry.

-- 
Thanks,
Sasha

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

[Sergey Senozhatsky]

On (26/08/10 15:47), Sean Christopherson wrote:
[..]
> 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:

I knew it!

>         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...

Sean, can I trouble you with a request to submit your cherry-pick to 6.1.y?