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

[sashiko-bot@kernel.org]

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 <seanjc@google.com>

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?

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