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

[Sean Christopherson <seanjc@google.com>]

On Tue, Nov 03, 2026, Ritesh Harjani wrote:
> Sean Christopherson <seanjc@google.com> writes:
> > +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()
> >> > @@ -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.

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.