Skip to contentSkip to conversation

KVM: arm64: nv: Shadow S2 life-cycle fixes

PATCH Superseded Marc Zyngier 4 participants 12 messages Aug 21 – Aug 24

Subsystems (3)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Marc Zyngier

author maintainer
#0 lore raw

v3 cover KVM: arm64: nv: Shadow S2 life-cycle fixes

This is the (hopefully) last revision for this set of fixes for the shadow S2 life cycle.

* From v2 [1]

- Fixed the nonsensical type used for allocating the nested S2 array, switching to kvmalloc_objs() in the process, guaranteeing some level of type checking

- Dropped the extra S2 freeing after Wei-Lin pointed out that we are guaranteed to run the MMU notifier release callback, ensuring that nothing is left behind

- Collected AB from Lorenzo, with thanks.

[1] https://lore.kernel.org/all/[email protected]/

Marc Zyngier (2):
KVM: arm64: nv: Fix life cycle of the nested_mmus array
KVM: arm64: nv: Delay freeing of shadow S2 structures until VM
destruction

 arch/arm64/include/asm/kvm_host.h   |   2 +-
 arch/arm64/include/asm/kvm_nested.h |   3 +-
 arch/arm64/kvm/arm.c                |   8 ++-
 arch/arm64/kvm/nested.c             | 100 ++++++++++++++--------------
 4 files changed, 58 insertions(+), 55 deletions(-)
Signature · 1 line
2.47.3
author maintainer
#1 lore raw

PATCH v3 1/2 KVM: arm64: nv: Fix life cycle of the nested_mmus array

The nested_mmus array holds the shadow page tables that are used when a guest is running a nested context. These structures are allocated on VCPU_INIT for whole guest, which implies that they may have to be relocated as the array grows.

Should a VCPU_INIT occur whilst a vcpu is actively running an L2 and that the allocation requires relocation, that vcpu will still be running with a pointer to the previous structure, which will have been freed.

Fix this by turning the array of structures to an array of pointers, which is now allocated at VM creation, sized to the absolute maximum that KVM can handle.

In turn, each VCPU_INIT contributes S2_MMU_PER_VCPU to the pool. No reallocation is ever performed, and the life cycle of each object is much clearer:

- the nested_mmus array is allocated in kvm_init_nested(), and freed in kvm_arch_destroy_vm()

- s2_mmu structures are allocated in kvm_vcpu_init_nested(), and freed on kvm_arch_flush_shadow_all()

Finally, the freeing of vcpu->arch.vncr_array is made consistent rather than being done on some failure paths, but not others.

---
 arch/arm64/include/asm/kvm_host.h   |  2 +-
 arch/arm64/include/asm/kvm_nested.h |  2 +-
 arch/arm64/kvm/arm.c                |  8 ++-
 arch/arm64/kvm/nested.c             | 93 +++++++++++++----------------
 4 files changed, 51 insertions(+), 54 deletions(-)
arch/arm64/include/asm/kvm_host.h +1 -1
index 108966a9db12b..08b2f24dc3c79 100644
@@ -322,7 +322,7 @@ struct kvm_arch {
 	 * Stage 2 paging state for VMs with nested S2 using a virtual
 	 * VMID.
 	 */
-	struct kvm_s2_mmu *nested_mmus;
+	struct kvm_s2_mmu **nested_mmus;
 	size_t nested_mmus_size;
 	int nested_mmus_next;
 
arch/arm64/include/asm/kvm_nested.h +1 -1
index 1ed7083358096..5b8edb2e8a87d 100644
@@ -66,7 +66,7 @@ static inline u64 translate_ttbr0_el2_to_ttbr0_el1(u64 ttbr0)
 
 extern bool forward_smc_trap(struct kvm_vcpu *vcpu);
 extern bool forward_debug_exception(struct kvm_vcpu *vcpu);
-extern void kvm_init_nested(struct kvm *kvm);
+extern int kvm_init_nested(struct kvm *kvm);
 extern int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu);
 extern void kvm_init_nested_s2_mmu(struct kvm_s2_mmu *mmu);
 extern struct kvm_s2_mmu *lookup_s2_mmu(struct kvm_vcpu *vcpu);
arch/arm64/kvm/arm.c +6 -2
index 50adfff75be82..7607173c1a40c 100644
@@ -223,8 +223,6 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	mutex_unlock(&kvm->lock);
 #endif
 
-	kvm_init_nested(kvm);
-
 	ret = kvm_share_hyp(kvm, kvm + 1);
 	if (ret)
 		return ret;
@@ -239,6 +237,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	if (ret)
 		goto err_free_cpumask;
 
+	ret = kvm_init_nested(kvm);
+	if (ret)
+		goto err_uninit_mmu;
+
 	if (is_protected_kvm_enabled()) {
 		/*
 		 * If any failures occur after this is successful, make sure to
@@ -267,6 +269,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 err_uninit_mmu:
 	kvm_uninit_stage2_mmu(kvm);
+	kvfree(kvm->arch.nested_mmus);
 err_free_cpumask:
 	free_cpumask_var(kvm->arch.supported_cpus);
 err_unshare_kvm:
@@ -324,6 +327,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 
 	kvm_unshare_hyp(kvm, kvm + 1);
 
+	kvfree(kvm->arch.nested_mmus);
 	kvm_arm_teardown_hypercalls(kvm);
 }
 
arch/arm64/kvm/nested.c +43 -50

171 lines not on this page · raw message

Signature · 1 line
2.47.3
#2 lore raw

Re: [PATCH v3 1/2] KVM: arm64: nv: Fix life cycle of the nested_mmus array

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Lockless iteration in kvm_arch_flush_shadow_all races with kvm_vcpu_init_nested, causing a garbage/NULL pointer dereference.

Pre-existing issues:
- [High] Memory leak of nested MMU stage-2 page tables (mmu->pgt) and per-CPU data when VM is destroyed after mmu_notifier_release has been bypassed. --

--- Patch [1]: [PATCH v3 1/2] KVM: arm64: nv: Fix life cycle of the nested_mmus array --- commit 2f4c64e5fe4ce7a7346ea83c7e69ecaa68ee9290

Author: Marc Zyngier <[email protected]>
Date:   Fri Aug 21 16:18:28 2026 +0000

KVM: arm64: nv: Fix life cycle of the nested_mmus array

This commit modifies the life cycle of the nested_mmus array by turning it from an array of structures into an array of pointers allocated at VM creation. It prevents issues where reallocation during VCPU_INIT could leave running vCPUs with freed pointers, ensuring S2 MMUs are incrementally allocated without reallocation.

Quoted 4 lines
arch/arm64/kvm/arm.c +0 -0
index 8b080804bc90b..b53219e048bdf 100644

[ ... ]

arch/arm64/kvm/arm.c void kvm_arch_destroy_vm(struct kvm *kvm) · line 343

Quoted 5 lines
arch/arm64/kvm/arm.c +1 -0
@@ -337,6 +340,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 
 	kvm_unshare_hyp(kvm, kvm + 1);
 
+	kvfree(kvm->arch.nested_mmus);

[Severity: High]
This is a pre-existing issue, but does this teardown path leak the nested MMU stage-2 page tables?

When a VM file descriptor is shared with a child process and the parent exits, the parent's exit_mmap() calls mmu_notifier_release() which unhashes the notifier. If the child process then initializes vCPUs, it allocates new nested MMU PGDs via init_nested_s2_mmu().

When the child exits, kvm_arch_destroy_vm() is called. Because the notifier is already unhashed, it bypasses the release callback, skipping kvm_arch_flush_shadow_all().

Since the cleanup path fails to call kvm_free_stage2_pgd() for the MMUs before freeing the nested_mmus array, the page tables (mmu->pgt) and per-CPU allocations (mmu->last_vcpu_ran) appear to be permanently leaked.

Quoted 7 lines
arch/arm64/kvm/arm.c +0 -0
 	kvm_arm_teardown_hypercalls(kvm);
 }
 
arch/arm64/kvm/nested.c +0 -0
index 17123f0b6daba..9070d5bc5af03 100644

[ ... ]

Quoted 1 line
arch/arm64/kvm/nested.c +0 -0
@@ -84,51 +89,40 @@ int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)

[ ... ]

arch/arm64/kvm/nested.c int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu)

Quoted 11 lines
arch/arm64/kvm/nested.c +4 -5
+		guard(write_lock)(&kvm->mmu_lock);
 
-		for (int i = kvm->arch.nested_mmus_size; i < num_mmus; i++)
-			kvm_free_stage2_pgd(&kvm->arch.nested_mmus[i]);
+		for (i = 0; i < S2_MMU_PER_VCPU; i++)
-		free_page((unsigned long)vcpu->arch.ctxt.vncr_array);
-		vcpu->arch.ctxt.vncr_array = NULL;
+			kvm->arch.nested_mmus[i + kvm->arch.nested_mmus_size] = &tmp[i];
 
-		return ret;
+		kvm->arch.nested_mmus_size += S2_MMU_PER_VCPU;

[Severity: High]
Could this result in a data race on weakly ordered architectures?

Here in kvm_vcpu_init_nested(), nested_mmus_size is incremented immediately after storing the new MMU pointers in the array. Since there are no memory barriers between the pointer stores and the size increment, the CPU could reorder the size increment to be visible before the array elements.

If kvm_arch_flush_shadow_all() concurrently iterates over the array locklessly (for example, during process exit via mmu_notifier_release), it could read the updated size but observe uninitialized array memory. This would result in reading a garbage pointer, leading to a kernel panic when it dereferences it in atomic_read(&mmu->refcnt).

Quoted 6 lines
arch/arm64/kvm/nested.c +0 -2
 	}
 
-	kvm->arch.nested_mmus_size = num_mmus;
-
 	return 0;
 }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
author maintainer
#3 lore raw

PATCH v3 2/2 KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

In reply to Marc Zyngier

We free the shadow S2 structures from kvm_arch_flush_shadow_all(), which is a Bad Idea(tm). Freeing the page tables is fair game (this is what this callback is for), but freeing the container that could still be referenced by another part of the system is not great.

Instead, grow separate destructors that gets called when we tear the VM down for good. From there, we can nuke both the individual MMUs as well as the global array that points to them, safe in the knowledge that the vcpus themselves have been destroyed already.

Fixes 4f128f8e1aaac ("KVM: arm64: nv: Support multiple nested Stage-2 mmu structures")
---
 arch/arm64/include/asm/kvm_nested.h |  1 +
 arch/arm64/kvm/arm.c                |  4 ++--
 arch/arm64/kvm/nested.c             | 15 ++++++++++-----
 3 files changed, 13 insertions(+), 7 deletions(-)
arch/arm64/include/asm/kvm_nested.h +1 -0
index 5b8edb2e8a87d..586026e859030 100644
@@ -67,6 +67,7 @@ static inline u64 translate_ttbr0_el2_to_ttbr0_el1(u64 ttbr0)
 extern bool forward_smc_trap(struct kvm_vcpu *vcpu);
 extern bool forward_debug_exception(struct kvm_vcpu *vcpu);
 extern int kvm_init_nested(struct kvm *kvm);
+extern void kvm_destroy_nested(struct kvm *kvm);
 extern int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu);
 extern void kvm_init_nested_s2_mmu(struct kvm_s2_mmu *mmu);
 extern struct kvm_s2_mmu *lookup_s2_mmu(struct kvm_vcpu *vcpu);
arch/arm64/kvm/arm.c +2 -2
index 7607173c1a40c..2b069c6440669 100644
@@ -269,7 +269,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 err_uninit_mmu:
 	kvm_uninit_stage2_mmu(kvm);
-	kvfree(kvm->arch.nested_mmus);
+	kvm_destroy_nested(kvm);
 err_free_cpumask:
 	free_cpumask_var(kvm->arch.supported_cpus);
 err_unshare_kvm:
@@ -327,7 +327,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 
 	kvm_unshare_hyp(kvm, kvm + 1);
 
-	kvfree(kvm->arch.nested_mmus);
+	kvm_destroy_nested(kvm);
 	kvm_arm_teardown_hypercalls(kvm);
 }
 
arch/arm64/kvm/nested.c +10 -5
index 05266f8b87304..00580ba6e8ba0 100644
@@ -55,6 +55,15 @@ int kvm_init_nested(struct kvm *kvm)
 	return kvm->arch.nested_mmus ? 0 : -ENOMEM;
 }
 
+void kvm_destroy_nested(struct kvm *kvm)
+{
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i+= S2_MMU_PER_VCPU)
+		kvfree(kvm->arch.nested_mmus[i]);
+
+	kvm->arch.nested_mmus_size = 0;
+	kvfree(kvm->arch.nested_mmus);
+}
+
 static int init_nested_s2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
 {
 	/*
@@ -1310,16 +1319,12 @@ void kvm_nested_s2_flush(struct kvm *kvm)
 
 void kvm_arch_flush_shadow_all(struct kvm *kvm)
 {
-	for (int i = kvm->arch.nested_mmus_size - 1; i >= 0; i--) {
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i++) {
 		struct kvm_s2_mmu *mmu = kvm->arch.nested_mmus[i];
 
 		if (!WARN_ON(atomic_read(&mmu->refcnt)))
 			kvm_free_stage2_pgd(mmu);
-
-		if ((i % S2_MMU_PER_VCPU) == 0)
-			kvfree(mmu);
 	}
-	kvm->arch.nested_mmus_size = 0;
 	kvm_uninit_stage2_mmu(kvm);
 }
 
Signature · 1 line
2.47.3
#4 lore raw

Re: [PATCH v3 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Preserving nested MMU containers with freed page tables leads to NULL pointer dereference.

Pre-existing issues:
- [High] Permanent memory leak of nested stage-2 page tables and per-CPU state on VM destruction.
- [High] Data race between lockless array iteration and array population causes uninitialized pointer dereference. --

--- Patch [2]: [PATCH v3 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction --- commit 46c2225152ae78e4009a13ad7c3bcc3e69fbab04
Author: Marc Zyngier <[email protected]>

KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

We free the shadow S2 structures from kvm_arch_flush_shadow_all(), which is a Bad Idea(tm). Freeing the page tables is fair game (this is what this callback is for), but freeing the container that could still be referenced by another part of the system is not great.

Instead, grow separate destructors that gets called when we tear the VM down for good. From there, we can nuke both the individual MMUs as well as the global array that points to them, safe in the knowledge that the vcpus themselves have been destroyed already.

Quoted 4 lines
arch/arm64/kvm/nested.c +0 -0
index 9070d5bc5af03..bc2b58e583b52 100644

[ ... ]

arch/arm64/kvm/nested.c int kvm_init_nested(struct kvm *kvm) · line 62

Quoted 8 lines
arch/arm64/kvm/nested.c +4 -0
@@ -56,6 +56,15 @@ int kvm_init_nested(struct kvm *kvm)
 	return kvm->arch.nested_mmus ? 0 : -ENOMEM;
 }
 
+void kvm_destroy_nested(struct kvm *kvm)
+{
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i+= S2_MMU_PER_VCPU)
+		kvfree(kvm->arch.nested_mmus[i]);

[Severity: High]
This is a pre-existing issue, but does this loop permanently leak the nested stage-2 page tables? The loop blindly calls kvfree() without first calling kvm_free_stage2_pgd() to recursively free the page tables and per-CPU allocations.

If a user creates a KVM VM, runs nested vCPUs to allocate stage-2 page tables, and then closes the KVM file descriptor, the KVM VM teardown calls kvm_destroy_nested(). Because it frees the container structures but forgets to call kvm_free_stage2_pgd(), the pgt and last_vcpu_ran allocations are permanently leaked.

Quoted 7 lines
arch/arm64/kvm/nested.c +5 -0
+
+	kvm->arch.nested_mmus_size = 0;
+	kvfree(kvm->arch.nested_mmus);
+}
+
 static int init_nested_s2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
 {

[ ... ]

arch/arm64/kvm/nested.c void kvm_nested_s2_flush(struct kvm *kvm) · line 1326

Quoted 9 lines
arch/arm64/kvm/nested.c +1 -1
@@ -1311,16 +1320,12 @@ void kvm_nested_s2_flush(struct kvm *kvm)
 
 void kvm_arch_flush_shadow_all(struct kvm *kvm)
 {
-	for (int i = kvm->arch.nested_mmus_size - 1; i >= 0; i--) {
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i++) {
 		struct kvm_s2_mmu *mmu = kvm->arch.nested_mmus[i];
 
 		if (!WARN_ON(atomic_read(&mmu->refcnt)))

[Severity: High]
This isn't a bug introduced by this patch, but can this lockless array iteration race with array population and cause an uninitialized pointer dereference?

While one thread executes KVM_ARM_VCPU_INIT and appends newly allocated MMU pointers to the nested_mmus array under mmu_lock in kvm_vcpu_init_nested():

	for (i = 0; i < S2_MMU_PER_VCPU; i++)
		kvm->arch.nested_mmus[i + kvm->arch.nested_mmus_size] = &tmp[i];

	kvm->arch.nested_mmus_size += S2_MMU_PER_VCPU;

Another process context (like OOM killer or thread exit) can trigger mmu_notifier_release -> kvm_arch_flush_shadow_all().

kvm_arch_flush_shadow_all() iterates over nested_mmus reading nested_mmus_size without mmu_lock or memory barriers. Because kvmalloc_objs does not zero memory, the lockless reader can observe an incremented size but a garbage pointer, causing a crash on atomic_read(&mmu->refcnt).

arch/arm64/kvm/nested.c void kvm_nested_s2_flush(struct kvm *kvm) · line 1326

Quoted 1 line
arch/arm64/kvm/nested.c +0 -0
 			kvm_free_stage2_pgd(mmu);

[Severity: High]
Since this patch now preserves the nested MMU containers, does this lead to a NULL pointer dereference?

The array elements are kept, but kvm_free_stage2_pgd(mmu) frees their page tables and sets mmu->pgt to NULL. If an application shares a KVM VM file descriptor across processes and the process holding the mm_struct exits, it fires mmu_notifier_release -> kvm_arch_flush_shadow_all().

If the surviving process calls KVM_RUN, get_s2_mmu_nested() can reuse the container since its refcnt is 0:

	s2_mmu = kvm->arch.nested_mmus[i % kvm->arch.nested_mmus_size];

	if (atomic_read(&s2_mmu->refcnt) == 0)
		break;

However, because there is no logic to reallocate the missing page table tree, a subsequent stage-2 fault will dereference mmu->pgt, panicking the kernel.

Quoted 7 lines
arch/arm64/kvm/nested.c +0 -4
-
-		if ((i % S2_MMU_PER_VCPU) == 0)
-			kvfree(mmu);
 	}
-	kvm->arch.nested_mmus_size = 0;
 	kvm_uninit_stage2_mmu(kvm);
 }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Reviewed-by on v3 2/2
#5 lore raw

Re: [PATCH v3 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

In reply to Marc Zyngier
Quoted 4 lines · Marc Zyngier

We free the shadow S2 structures from kvm_arch_flush_shadow_all(), which is a Bad Idea(tm). Freeing the page tables is fair game (this is what this callback is for), but freeing the container that could still be referenced by another part of the system is not great.

Yes.

Quoted 8 lines

Instead, grow separate destructors that gets called when we tear the VM down for good. From there, we can nuke both the individual MMUs as well as the global array that points to them, safe in the knowledge that the vcpus themselves have been destroyed already.

Fixes 4f128f8e1aaac ("KVM: arm64: nv: Support multiple nested Stage-2 mmu structures")

LGTM so:

A couple thoughts/questions below.

Quoted 62 lines
---
 arch/arm64/include/asm/kvm_nested.h |  1 +
 arch/arm64/kvm/arm.c                |  4 ++--
 arch/arm64/kvm/nested.c             | 15 ++++++++++-----
 3 files changed, 13 insertions(+), 7 deletions(-)
arch/arm64/include/asm/kvm_nested.h +1 -0
index 5b8edb2e8a87d..586026e859030 100644
@@ -67,6 +67,7 @@ static inline u64 translate_ttbr0_el2_to_ttbr0_el1(u64 ttbr0)
 extern bool forward_smc_trap(struct kvm_vcpu *vcpu);
 extern bool forward_debug_exception(struct kvm_vcpu *vcpu);
 extern int kvm_init_nested(struct kvm *kvm);
+extern void kvm_destroy_nested(struct kvm *kvm);
 extern int kvm_vcpu_init_nested(struct kvm_vcpu *vcpu);
 extern void kvm_init_nested_s2_mmu(struct kvm_s2_mmu *mmu);
 extern struct kvm_s2_mmu *lookup_s2_mmu(struct kvm_vcpu *vcpu);
arch/arm64/kvm/arm.c +2 -2
index 7607173c1a40c..2b069c6440669 100644
@@ -269,7 +269,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 
 err_uninit_mmu:
 	kvm_uninit_stage2_mmu(kvm);
-	kvfree(kvm->arch.nested_mmus);
+	kvm_destroy_nested(kvm);
 err_free_cpumask:
 	free_cpumask_var(kvm->arch.supported_cpus);
 err_unshare_kvm:
@@ -327,7 +327,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 
 	kvm_unshare_hyp(kvm, kvm + 1);
 
-	kvfree(kvm->arch.nested_mmus);
+	kvm_destroy_nested(kvm);
 	kvm_arm_teardown_hypercalls(kvm);
 }
 
arch/arm64/kvm/nested.c +9 -0
index 05266f8b87304..00580ba6e8ba0 100644
@@ -55,6 +55,15 @@ int kvm_init_nested(struct kvm *kvm)
 	return kvm->arch.nested_mmus ? 0 : -ENOMEM;
 }
 
+void kvm_destroy_nested(struct kvm *kvm)
+{
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i+= S2_MMU_PER_VCPU)
+		kvfree(kvm->arch.nested_mmus[i]);
+
+	kvm->arch.nested_mmus_size = 0;
+	kvfree(kvm->arch.nested_mmus);
+}
+
 static int init_nested_s2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu)
 {
 	/*

arch/arm64/kvm/nested.c void kvm_nested_s2_flush(struct kvm *kvm) · line 1322

Quoted 5 lines
arch/arm64/kvm/nested.c +1 -1
@@ -1310,16 +1319,12 @@ void kvm_nested_s2_flush(struct kvm *kvm)
 
 void kvm_arch_flush_shadow_all(struct kvm *kvm)
 {
-	for (int i = kvm->arch.nested_mmus_size - 1; i >= 0; i--) {
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i++) {

Hmm why this was in reverse before? :) I guess some product of the kvfree() bit or maybe something else?

arch/arm64/kvm/nested.c void kvm_nested_s2_flush(struct kvm *kvm) · line 1326

Quoted 4 lines
arch/arm64/kvm/nested.c +0 -0
 		struct kvm_s2_mmu *mmu = kvm->arch.nested_mmus[i];
 
 		if (!WARN_ON(atomic_read(&mmu->refcnt)))
 			kvm_free_stage2_pgd(mmu);

Yeah I think the mmu write lock taken by the function and the fact mmus are marked invalid here (VTTBR_CNP_BIT set kvm_free_stage2_pgd() -> kvm_init_nested_s2_mmu()) makes this entirely fine to do here, as you say.

arch/arm64/kvm/nested.c void kvm_nested_s2_flush(struct kvm *kvm) · line 1322

Quoted 5 lines
arch/arm64/kvm/nested.c +0 -4
-
-		if ((i % S2_MMU_PER_VCPU) == 0)
-			kvfree(mmu);
 	}
-	kvm->arch.nested_mmus_size = 0;

And, neatly, this makes the patch simply take away the bad bits to do here and to do them in the correct place.

Quoted 6 lines
arch/arm64/kvm/nested.c +0 -1
 	kvm_uninit_stage2_mmu(kvm);
 }
 
--

2.47.3

--
Cheers, Lorenzo

author maintainer
#6 lore raw

Re: [PATCH v3 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

Quoted 15 lines · Lorenzo Stoakes (ARM)
Quoted 4 lines · Marc Zyngier

We free the shadow S2 structures from kvm_arch_flush_shadow_all(), which is a Bad Idea(tm). Freeing the page tables is fair game (this is what this callback is for), but freeing the container that could still be referenced by another part of the system is not great.

Yes.

Quoted 6 lines

Instead, grow separate destructors that gets called when we tear the VM down for good. From there, we can nuke both the individual MMUs as well as the global array that points to them, safe in the knowledge that the vcpus themselves have been destroyed already.

Quoted 6 lines
Quoted 2 lines
Fixes 4f128f8e1aaac ("KVM: arm64: nv: Support multiple nested Stage-2 mmu structures")

LGTM so:

Thanks.

Quoted 1 line

A couple thoughts/questions below.

[...]

Quoted 9 lines

arch/arm64/kvm/nested.c void kvm_nested_s2_flush(struct kvm *kvm) · line 1322

Quoted 6 lines
arch/arm64/kvm/nested.c +1 -1
@@ -1310,16 +1319,12 @@ void kvm_nested_s2_flush(struct kvm *kvm)
 
 void kvm_arch_flush_shadow_all(struct kvm *kvm)
 {
-	for (int i = kvm->arch.nested_mmus_size - 1; i >= 0; i--) {
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i++) {

Hmm why this was in reverse before? :) I guess some product of the kvfree() bit or maybe something else?

We allocate s2_mmus in S2_MMU_PER_VCPU chunks. Which means that it complicates the freeing of the these structures, as they can only be freed once all S2 PTs of that chunk have been freed. We have three options:

- scan forward, and use complicated logic to work out that you have freed the last PTs of a chunk, freeing with a negative offset from the current point in the loop. Works, but hard to reason about in fewer than 3 seconds.

- scan backward, use simpler logic to ensure you have reached the beginning of a chunk, nuke it.

- have two loops, one for the PTs, one for the MMUs. That's what we end-up with due to the different garbage collection phases.

Cheers,

	M.
Signature · 1 line
Without deviation from the norm, progress is not possible.
#7 lore raw

Re: [PATCH v3 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

Quoted 22 lines · Marc Zyngier
Quoted 9 lines · Lorenzo Stoakes (ARM)

arch/arm64/kvm/nested.c void kvm_nested_s2_flush(struct kvm *kvm) · line 1322

Quoted 6 lines
arch/arm64/kvm/nested.c +1 -1
@@ -1310,16 +1319,12 @@ void kvm_nested_s2_flush(struct kvm *kvm)
 
 void kvm_arch_flush_shadow_all(struct kvm *kvm)
 {
-	for (int i = kvm->arch.nested_mmus_size - 1; i >= 0; i--) {
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i++) {

Hmm why this was in reverse before? :) I guess some product of the kvfree() bit or maybe something else?

We allocate s2_mmus in S2_MMU_PER_VCPU chunks. Which means that it complicates the freeing of the these structures, as they can only be freed once all S2 PTs of that chunk have been freed. We have three options:

- scan forward, and use complicated logic to work out that you have freed the last PTs of a chunk, freeing with a negative offset from the current point in the loop. Works, but hard to reason about in fewer than 3 seconds.

Quoted 5 lines

- scan backward, use simpler logic to ensure you have reached the beginning of a chunk, nuke it.

- have two loops, one for the PTs, one for the MMUs. That's what we end-up with due to the different garbage collection phases.

Ah I see, so tear down page tables of the 2nd of the pair, then after doing it for the 1st of the pair free the chunk, makes sense.

Quoted 7 lines

Cheers,

	M.

--
Without deviation from the norm, progress is not possible.

--
Cheers, Lorenzo

#8 lore raw

Re: [PATCH v3 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

In reply to Marc Zyngier

On Fri, Aug 21, 2026 at 05:18:29PM +0100, Marc Zyngier wrote:

[...]

arch/arm64/kvm/nested.c int kvm_init_nested(struct kvm *kvm) · line 60

Quoted 3 lines
arch/arm64/kvm/nested.c +3 -0
+void kvm_destroy_nested(struct kvm *kvm)
+{
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i+= S2_MMU_PER_VCPU)

nit: ^ missing space

Thanks,
Wei-Lin Chang

Quoted 6 lines
arch/arm64/kvm/nested.c +6 -0
+		kvfree(kvm->arch.nested_mmus[i]);
+
+	kvm->arch.nested_mmus_size = 0;
+	kvfree(kvm->arch.nested_mmus);
+}
+

[...]

#9 lore raw

Re: [PATCH v3 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

Quoted 9 lines · Wei-Lin Chang

On Fri, Aug 21, 2026 at 05:18:29PM +0100, Marc Zyngier wrote:

[...]

arch/arm64/kvm/nested.c int kvm_init_nested(struct kvm *kvm) · line 60

Quoted 3 lines
arch/arm64/kvm/nested.c +3 -0
+void kvm_destroy_nested(struct kvm *kvm)
+{
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i+= S2_MMU_PER_VCPU)

nit: ^ missing space

Ack will fix on respin :)

Quoted 4 lines

Thanks,
Wei-Lin Chang

Quoted 8 lines
Quoted 6 lines
arch/arm64/kvm/nested.c +6 -0
+		kvfree(kvm->arch.nested_mmus[i]);
+
+	kvm->arch.nested_mmus_size = 0;
+	kvfree(kvm->arch.nested_mmus);
+}
+

[...]

--
Cheers, Lorenzo

#10 lore raw

Re: [PATCH v3 2/2] KVM: arm64: nv: Delay freeing of shadow S2 structures until VM destruction

Quoted 5 lines · Lorenzo Stoakes (ARM)
Quoted 4 lines · Wei-Lin Chang

On Fri, Aug 21, 2026 at 05:18:29PM +0100, Marc Zyngier wrote:

[...]

Quoted 7 lines
Quoted 5 lines

arch/arm64/kvm/nested.c int kvm_init_nested(struct kvm *kvm) · line 60

Quoted 3 lines
arch/arm64/kvm/nested.c +3 -0
+void kvm_destroy_nested(struct kvm *kvm)
+{
+	for (int i = 0; i < kvm->arch.nested_mmus_size; i+= S2_MMU_PER_VCPU)

nit: ^ missing space

Ack will fix on respin :)

Sorry I have not had my coffee yet this morning :)) this is Marc's series and not mine... :P Disregard...

--
Cheers, Lorenzo

Reviewed-by on the v3 series
#11 lore raw
In reply to Marc Zyngier
Quoted 22 lines · Marc Zyngier

This is the (hopefully) last revision for this set of fixes for the shadow S2 life cycle.

* From v2 [1]

- Fixed the nonsensical type used for allocating the nested S2 array, switching to kvmalloc_objs() in the process, guaranteeing some level of type checking

- Dropped the extra S2 freeing after Wei-Lin pointed out that we are guaranteed to run the MMU notifier release callback, ensuring that nothing is left behind

- Collected AB from Lorenzo, with thanks.

[1] https://lore.kernel.org/all/[email protected]/

Marc Zyngier (2):
KVM: arm64: nv: Fix life cycle of the nested_mmus array
KVM: arm64: nv: Delay freeing of shadow S2 structures until VM
destruction

Quoted 9 lines
 arch/arm64/include/asm/kvm_host.h   |   2 +-
 arch/arm64/include/asm/kvm_nested.h |   3 +-
 arch/arm64/kvm/arm.c                |   8 ++-
 arch/arm64/kvm/nested.c             | 100 ++++++++++++++--------------
 4 files changed, 58 insertions(+), 55 deletions(-)
Signature · 1 line
2.47.3