KVM: arm64: nv: Skip vCPUs without a pseudo-TLB in invalidate_vncr_va()

vncr_tlb is not allocated before a vCPU runs for the first time, so vcpu->arch.vncr_tlb is NULL for a vCPU that has been created but not yet run. Code that iterates over every vCPU's pseudo-TLB must skip those.

invalidate_vncr_va() iterates over the vCPUs with kvm_for_each_vcpu() and dereferences vt->valid without checking whether vncr_tlb is NULL.

While iterating, skip vCPUs whose pseudo-TLB has not been allocated.

Fixes 4ffa72ad8f37 ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")

Signed-off-by Hyunwoo Kim <[email protected]>

---
 arch/arm64/kvm/nested.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm64/kvm/nested.c +0 -0

index 6f7bc9a9992e..063e079d1d1a 100644

Quoted 21 lines · Hyunwoo Kim

vncr_tlb is not allocated before a vCPU runs for the first time, so vcpu->arch.vncr_tlb is NULL for a vCPU that has been created but not yet run. Code that iterates over every vCPU's pseudo-TLB must skip those.

invalidate_vncr_va() iterates over the vCPUs with kvm_for_each_vcpu() and dereferences vt->valid without checking whether vncr_tlb is NULL.

While iterating, skip vCPUs whose pseudo-TLB has not been allocated.

Fixes 4ffa72ad8f37 ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")

Signed-off-by Hyunwoo Kim <[email protected]>

---
 arch/arm64/kvm/nested.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm64/kvm/nested.c +0 -0

index 6f7bc9a9992e..063e079d1d1a 100644

arch/arm64/kvm/nested.c static void invalidate_vncr_va(struct kvm *kvm, · line 978

Quoted 10 lines

arch/arm64/kvm/nested.c +4 -0

@@ -969,6 +969,10 @@ static void invalidate_vncr_va(struct kvm *kvm,
 		struct vncr_tlb *vt = vcpu->arch.vncr_tlb;
 		u64 va_start, va_end, va_size;
 
+		/* Skip vCPUs whose pseudo-TLB hasn't been allocated yet */
+		if (!vt)
+			continue;
+
 		if (!vt->valid)
 			continue;
 

This looks correct and matches what we already have for invalidate_vncr_ipa().

But I think this misses the opportunity to squash a whole class of similar bugs, should we ever have the need for another function that iterates over all *valid* VNCR pseudo-TLBs.

Since I'm on a train and have nothing better to do, I've written the following hack.

Thoughts?

	M.

arch/arm64/kvm/nested.c +0 -0

index 38f672e940878..f0a9f81a08302 100644
@@ -897,9 +897,21 @@ static void invalidate_vncr(struct vncr_tlb *vt)
 		clear_fixmap(vncr_fixmap(vt->cpu));

arch/arm64/kvm/nested.c +7 -21

+		if (((tlbp) = vcpu->arch.vncr_tlb) &&	\
+		    (tlbp)->valid)
+
 static void kvm_invalidate_vncr_ipa(struct kvm *kvm, u64 start, u64 end)
 {
 	struct kvm_vcpu *vcpu;
+	struct vncr_tlb *vt;
 	unsigned long i;
 
 	lockdep_assert_held_write(&kvm->mmu_lock);

@@ -907,24 +919,9 @@ static void kvm_invalidate_vncr_ipa(struct kvm *kvm, u64 start, u64 end)
 	if (!kvm_has_feat(kvm, ID_AA64MMFR4_EL1, NV_frac, NV2_ONLY))
 		return;
 
-	kvm_for_each_vcpu(i, vcpu, kvm) {
-		struct vncr_tlb *vt = vcpu->arch.vncr_tlb;
+	kvm_for_each_vncr_tlb(i, vcpu, vt, kvm) {
 		u64 ipa_start, ipa_end, ipa_size;
 
-		/*
-		 * Careful here: We end-up here from an MMU notifier,
-		 * and this can race against a vcpu not being onlined
-		 * yet, without the pseudo-TLB being allocated.
-		 *
-		 * Skip those, as they obviously don't participate in
-		 * the invalidation at this stage.
-		 */
-		if (!vt)
-			continue;
-
-		if (!vt->valid)
-			continue;
-
 		ipa_size = ttl_to_size(pgshift_level_to_ttl(vt->wi.pgshift,
 							    vt->wr.level));
 		ipa_start = vt->wr.pa & ~(ipa_size - 1);

@@ -954,17 +951,14 @@ static void invalidate_vncr_va(struct kvm *kvm,
 			       struct s1e2_tlbi_scope *scope)
 {
 	struct kvm_vcpu *vcpu;
+	struct vncr_tlb *vt;
 	unsigned long i;
 
 	lockdep_assert_held_write(&kvm->mmu_lock);
 
-	kvm_for_each_vcpu(i, vcpu, kvm) {
-		struct vncr_tlb *vt = vcpu->arch.vncr_tlb;
+	kvm_for_each_vncr_tlb(i, vcpu, vt, kvm) {
 		u64 va_start, va_end, va_size;
 
-		if (!vt->valid)
-			continue;
-
 		va_size = ttl_to_size(pgshift_level_to_ttl(vt->wi.pgshift,
 							   vt->wr.level));
 		va_start = vt->gva & ~(va_size - 1);

Signature · 2 lines

-- 
Jazz isn't dead. It just smells funny.

Quoted 44 lines · Marc Zyngier

Quoted 21 lines · Hyunwoo Kim

vncr_tlb is not allocated before a vCPU runs for the first time, so vcpu->arch.vncr_tlb is NULL for a vCPU that has been created but not yet run. Code that iterates over every vCPU's pseudo-TLB must skip those.

invalidate_vncr_va() iterates over the vCPUs with kvm_for_each_vcpu() and dereferences vt->valid without checking whether vncr_tlb is NULL.

While iterating, skip vCPUs whose pseudo-TLB has not been allocated.

Fixes 4ffa72ad8f37 ("KVM: arm64: nv: Add S1 TLB invalidation primitive for VNCR_EL2")

Signed-off-by Hyunwoo Kim <[email protected]>

---
 arch/arm64/kvm/nested.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/arm64/kvm/nested.c +0 -0

index 6f7bc9a9992e..063e079d1d1a 100644

arch/arm64/kvm/nested.c static void invalidate_vncr_va(struct kvm *kvm, · line 978

Quoted 10 lines

arch/arm64/kvm/nested.c +4 -0

@@ -969,6 +969,10 @@ static void invalidate_vncr_va(struct kvm *kvm,
 		struct vncr_tlb *vt = vcpu->arch.vncr_tlb;
 		u64 va_start, va_end, va_size;
 
+		/* Skip vCPUs whose pseudo-TLB hasn't been allocated yet */
+		if (!vt)
+			continue;
+
 		if (!vt->valid)
 			continue;
 

This looks correct and matches what we already have for invalidate_vncr_ipa().

But I think this misses the opportunity to squash a whole class of similar bugs, should we ever have the need for another function that iterates over all *valid* VNCR pseudo-TLBs.

Since I'm on a train and have nothing better to do, I've written the following hack.

Quoted 1 line

Thoughts?

Looks like a good direction to me. I confirmed it fixes the issue (as expected).

How about you submit this patch yourself?

arch/arm64/kvm/nested.c static void invalidate_vncr(struct vncr_tlb *vt) · line 907

Quoted 2 lines

arch/arm64/kvm/nested.c +2 -0

+#define kvm_for_each_vncr_tlb(idx, vcpup, tlbp, kvm)	\
+	kvm_for_each_vcpu(idx, vcpu, kvm)		\

Maybe vcpu -> vcpup?