Sean Christopherson <[email protected]> writes:
> +Gautam and PPC lists/maintainers
>
> On Fri, Oct 30, 2026, [email protected] 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 <[email protected]>
>>
>> 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().
>>
>> > 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
>>
>> [ ... ]
>>
>> > @@ -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.
> 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
> 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/[email protected]
>
>> 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/[email protected]?part=12
>>
