Re: [PATCH v4 12/23] KVM: Destroy memslots immediately after mmu_notifiers are unregistered

Started by Sean Christopherson on Nov 2, 2026 · 4 messages from 2 authors · Last reply Nov 4, 2026

Subsystems (3)

Maintainers (2)

open the first message on its own page

Message details and related content
+Gautam and PPC lists/maintainers

On Fri, Oct 30, 2026, sashiko-bot@kernel.org wrote:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: PPC: Book3S HV: rmap_nested memory leak in kvmhv_release_all_nested()
--

commit 9c25a1b4a76e8526eb8a2b11e1b9e63893b0de80
Author: Sean Christopherson [off-list ref]

KVM: Destroy memslots immediately after mmu_notifiers are unregistered

This commit replaces active memslots with an empty memslot set and frees all
memslots immediately after unregistering the MMU notifier in kvm_destroy_vm(),
prior to calling kvm_arch_destroy_vm().
quoted
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index fa548b12228ff..2eda12a52bf4b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
[ ... ]
quoted
@@ -1310,12 +1329,10 @@ static void kvm_destroy_vm(struct kvm *kvm)
 		kvm->mn_active_invalidate_count = 0;
 	else
 		WARN_ON(kvm->mmu_invalidate_in_progress);
+	kvm_destroy_memslots(kvm);
+
 	kvm_arch_destroy_vm(kvm);
 	kvm_destroy_devices(kvm);
-	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
-		kvm_free_memslots(kvm, &kvm->__memslots[i][0]);
-		kvm_free_memslots(kvm, &kvm->__memslots[i][1]);
-	}
[Severity: Medium]
Does hoisting kvm_destroy_memslots() above kvm_arch_destroy_vm() leak
struct rmap_nested allocations in slot->arch.rmap on PowerPC Book3S HV when
destroying a VM with nested virtualization enabled?
I don't think so?  Patch 3[*], "KVM: PPC: Flush/zap all memslots on kvm_arch_flush_shadow_all()",
should free the rmaps in response to kvm_arch_flush_shadow_all().   From what I
can tell, the page tables have to be in radix mode, and so the rmaps are freed via:

  kvmppc_radix_flush_memslot()
  |
  -> kvmppc_unmap_pte()
     |
     -> kvmhv_remove_nest_rmap_range()
        |
        -> kvmhv_remove_nest_rmap_list()

And if that doesn't work, it means memslot deletion on an active VM is completely
broken.

However, it means the memslot code in kvmhv_release_all_nested() is dead code, so
this as follow-up to patch 3?  Or maybe folded in?
diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
index a6ff42d7666c..27985210f972 100644
--- a/arch/powerpc/kvm/book3s_hv_nested.c
+++ b/arch/powerpc/kvm/book3s_hv_nested.c
@@ -802,8 +802,6 @@ void kvmhv_release_all_nested(struct kvm *kvm)
 	int lpid;
 	struct kvm_nested_guest *gp;
 	struct kvm_nested_guest *freelist = NULL;
-	struct kvm_memory_slot *memslot;
-	int srcu_idx, bkt;
 
 	spin_lock(&kvm->mmu_lock);
 	idr_for_each_entry(&kvm->arch.kvm_nested_guest_idr, gp, lpid) {
@@ -820,11 +818,6 @@ void kvmhv_release_all_nested(struct kvm *kvm)
 		freelist = gp->next;
 		kvmhv_release_nested(gp);
 	}
-
-	srcu_idx = srcu_read_lock(&kvm->srcu);
-	kvm_for_each_memslot(memslot, bkt, kvm_memslots(kvm))
-		kvmhv_free_memslot_nest_rmap(memslot);
-	srcu_read_unlock(&kvm->srcu, srcu_idx);
 }
 
 /* caller must hold gp->tlb_lock */
			

[*] https://lore.kernel.org/all/20261030212112.2598640-4-seanjc@google.com
In virt/kvm/kvm_main.c, kvm_destroy_memslots() replaces kvm->memslots[i]
with kvm_empty_memslots and frees each memslot via
kvm_free_memslot() -> kvm_arch_free_memslot() -> kvmppc_core_free_memslot_hv()
in arch/powerpc/kvm/book3s_hv.c:

    static void kvmppc_core_free_memslot_hv(struct kvm_memory_slot *slot)
    {
            vfree(slot->arch.rmap);
            slot->arch.rmap = NULL;
    }

which frees the slot->arch.rmap array without freeing the linked
struct rmap_nested entries.

Previously, those entries were freed before memslot destruction via
kvm_arch_destroy_vm() -> kvmppc_core_destroy_vm_hv() ->
kvmhv_release_all_nested() in arch/powerpc/kvm/book3s_hv_nested.c:

    srcu_idx = srcu_read_lock(&kvm->srcu);
    kvm_for_each_memslot(memslot, bkt, kvm_memslots(kvm))
            kvmhv_free_memslot_nest_rmap(memslot);
    srcu_read_unlock(&kvm->srcu, srcu_idx);

With kvm_destroy_memslots() running before kvm_arch_destroy_vm(),
kvm_memslots(kvm) now returns kvm_empty_memslots and the memslots have
already been freed when kvmhv_release_all_nested() runs, so
kvmhv_free_memslot_nest_rmap() is never called. Any struct rmap_nested
entries remaining in slot->arch.rmap at teardown (for example, when
KVMPPC_SECURE_INIT_DONE causes kvmppc_radix_flush_memslot() to return early,
or when 4KB PTEs were replaced by a huge page in kvmppc_unmap_free_pte())
will be leaked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030212112.2598640-1-seanjc@google.com?part=12
Message details and related content
+Ritesh as well

On Mon, Nov 02, 2026, Sean Christopherson wrote:
+Gautam and PPC lists/maintainers

On Fri, Oct 30, 2026, sashiko-bot@kernel.org wrote:
quoted
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: PPC: Book3S HV: rmap_nested memory leak in kvmhv_release_all_nested()
--

commit 9c25a1b4a76e8526eb8a2b11e1b9e63893b0de80
Author: Sean Christopherson [off-list ref]

KVM: Destroy memslots immediately after mmu_notifiers are unregistered

This commit replaces active memslots with an empty memslot set and frees all
memslots immediately after unregistering the MMU notifier in kvm_destroy_vm(),
prior to calling kvm_arch_destroy_vm().
quoted
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index fa548b12228ff..2eda12a52bf4b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
[ ... ]
quoted
@@ -1310,12 +1329,10 @@ static void kvm_destroy_vm(struct kvm *kvm)
 		kvm->mn_active_invalidate_count = 0;
 	else
 		WARN_ON(kvm->mmu_invalidate_in_progress);
+	kvm_destroy_memslots(kvm);
+
 	kvm_arch_destroy_vm(kvm);
 	kvm_destroy_devices(kvm);
-	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
-		kvm_free_memslots(kvm, &kvm->__memslots[i][0]);
-		kvm_free_memslots(kvm, &kvm->__memslots[i][1]);
-	}
[Severity: Medium]
Does hoisting kvm_destroy_memslots() above kvm_arch_destroy_vm() leak
struct rmap_nested allocations in slot->arch.rmap on PowerPC Book3S HV when
destroying a VM with nested virtualization enabled?

84 more quoted lines

Message details and related content
Sean Christopherson [off-list ref] writes:
+Gautam and PPC lists/maintainers

On Fri, Oct 30, 2026, sashiko-bot@kernel.org wrote:
quoted
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: PPC: Book3S HV: rmap_nested memory leak in kvmhv_release_all_nested()
--

commit 9c25a1b4a76e8526eb8a2b11e1b9e63893b0de80
Author: Sean Christopherson [off-list ref]

KVM: Destroy memslots immediately after mmu_notifiers are unregistered

This commit replaces active memslots with an empty memslot set and frees all
memslots immediately after unregistering the MMU notifier in kvm_destroy_vm(),
prior to calling kvm_arch_destroy_vm().
quoted
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index fa548b12228ff..2eda12a52bf4b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
[ ... ]
quoted
@@ -1310,12 +1329,10 @@ static void kvm_destroy_vm(struct kvm *kvm)
 		kvm->mn_active_invalidate_count = 0;
 	else
 		WARN_ON(kvm->mmu_invalidate_in_progress);
+	kvm_destroy_memslots(kvm);
+
 	kvm_arch_destroy_vm(kvm);
 	kvm_destroy_devices(kvm);
-	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
-		kvm_free_memslots(kvm, &kvm->__memslots[i][0]);
-		kvm_free_memslots(kvm, &kvm->__memslots[i][1]);
-	}
[Severity: Medium]
Does hoisting kvm_destroy_memslots() above kvm_arch_destroy_vm() leak
struct rmap_nested allocations in slot->arch.rmap on PowerPC Book3S HV when
destroying a VM with nested virtualization enabled?

13 more quoted lines

That's correct. Maybe, I think Sashiko didn't consider patch-3 of the series
during review of this patch-12.
And if that doesn't work, it means memslot deletion on an active VM is completely
broken.

However, it means the memslot code in kvmhv_release_all_nested() is
dead code,
Not really. I do see a caller of this, for when we switch from radix
to HPT. So we still need it.

INCOMING CALLS
- f kvmhv_release_all_nested
  - f kvmppc_switch_mmu_to_hpt    [arch/powerpc/kvm/book3s_hv.c:5591]
    + f kvmppc_alloc_reset_hpt    [arch/powerpc/kvm/book3s_64_mmu_hv.c:144]
    + f kvmhv_configure_mmu       [arch/powerpc/kvm/book3s_hv.c:6330]
  + f kvmppc_core_destroy_vm_hv   [arch/powerpc/kvm/book3s_hv.c:5930]



So FWIW patch-12 as such looks good to me from PowerPC perspective,
since those rmap entries will get freed up as part of the
mmu_notifier_unregister() (after patch-3). Agreed that Sashiko
comment does not look correct.

Thanks for looking into this.

-ritesh

quoted hunk
so this as follow-up to patch 3?  Or maybe folded in?
diff --git a/arch/powerpc/kvm/book3s_hv_nested.c b/arch/powerpc/kvm/book3s_hv_nested.c
index a6ff42d7666c..27985210f972 100644
--- a/arch/powerpc/kvm/book3s_hv_nested.c
+++ b/arch/powerpc/kvm/book3s_hv_nested.c
@@ -802,8 +802,6 @@ void kvmhv_release_all_nested(struct kvm *kvm)
 	int lpid;
 	struct kvm_nested_guest *gp;
 	struct kvm_nested_guest *freelist = NULL;
-	struct kvm_memory_slot *memslot;
-	int srcu_idx, bkt;
 
 	spin_lock(&kvm->mmu_lock);
 	idr_for_each_entry(&kvm->arch.kvm_nested_guest_idr, gp, lpid) {
@@ -820,11 +818,6 @@ void kvmhv_release_all_nested(struct kvm *kvm)
 		freelist = gp->next;
 		kvmhv_release_nested(gp);
 	}
-
-	srcu_idx = srcu_read_lock(&kvm->srcu);
-	kvm_for_each_memslot(memslot, bkt, kvm_memslots(kvm))
-		kvmhv_free_memslot_nest_rmap(memslot);
-	srcu_read_unlock(&kvm->srcu, srcu_idx);
 }
 
 /* caller must hold gp->tlb_lock */
			

[*] https://lore.kernel.org/all/20261030212112.2598640-4-seanjc@google.com
quoted
In virt/kvm/kvm_main.c, kvm_destroy_memslots() replaces kvm->memslots[i]
with kvm_empty_memslots and frees each memslot via
kvm_free_memslot() -> kvm_arch_free_memslot() -> kvmppc_core_free_memslot_hv()
in arch/powerpc/kvm/book3s_hv.c:

    static void kvmppc_core_free_memslot_hv(struct kvm_memory_slot *slot)
    {
            vfree(slot->arch.rmap);
            slot->arch.rmap = NULL;

26 more quoted lines

Message details and related content
On Tue, Nov 03, 2026, Ritesh Harjani wrote:
Sean Christopherson [off-list ref] writes:
quoted
+Gautam and PPC lists/maintainers
On Fri, Oct 30, 2026, sashiko-bot@kernel.org wrote:
quoted
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: PPC: Book3S HV: rmap_nested memory leak in kvmhv_release_all_nested()
quoted
@@ -1310,12 +1329,10 @@ static void kvm_destroy_vm(struct kvm *kvm)
 		kvm->mn_active_invalidate_count = 0;
 	else
 		WARN_ON(kvm->mmu_invalidate_in_progress);
+	kvm_destroy_memslots(kvm);
+
 	kvm_arch_destroy_vm(kvm);
 	kvm_destroy_devices(kvm);
-	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
-		kvm_free_memslots(kvm, &kvm->__memslots[i][0]);
-		kvm_free_memslots(kvm, &kvm->__memslots[i][1]);
-	}
[Severity: Medium]
Does hoisting kvm_destroy_memslots() above kvm_arch_destroy_vm() leak
struct rmap_nested allocations in slot->arch.rmap on PowerPC Book3S HV when
destroying a VM with nested virtualization enabled?
I don't think so?  Patch 3[*], "KVM: PPC: Flush/zap all memslots on kvm_arch_flush_shadow_all()",
should free the rmaps in response to kvm_arch_flush_shadow_all().   From what I
can tell, the page tables have to be in radix mode, and so the rmaps are freed via:

  kvmppc_radix_flush_memslot()
  |
  -> kvmppc_unmap_pte()
     |
     -> kvmhv_remove_nest_rmap_range()
        |
        -> kvmhv_remove_nest_rmap_list()
That's correct. Maybe, I think Sashiko didn't consider patch-3 of the series
during review of this patch-12.
quoted
And if that doesn't work, it means memslot deletion on an active VM is completely

7 more quoted lines

Oh, nice, I somehow missed that.  Yay, less work to do!
INCOMING CALLS
- f kvmhv_release_all_nested
  - f kvmppc_switch_mmu_to_hpt    [arch/powerpc/kvm/book3s_hv.c:5591]
    + f kvmppc_alloc_reset_hpt    [arch/powerpc/kvm/book3s_64_mmu_hv.c:144]
    + f kvmhv_configure_mmu       [arch/powerpc/kvm/book3s_hv.c:6330]
  + f kvmppc_core_destroy_vm_hv   [arch/powerpc/kvm/book3s_hv.c:5930]



So FWIW patch-12 as such looks good to me from PowerPC perspective,
since those rmap entries will get freed up as part of the
mmu_notifier_unregister() (after patch-3). Agreed that Sashiko
comment does not look correct.
Keyboard shortcuts
hback out one level
jnext message in thread / next series-diff position
kprevious message in thread / previous series-diff position
ldrill in
Escclose help / fold thread tree
?toggle this help