[RFC PATCH 30/55] KVM: planes: Unwind published state on KVM_CREATE_PLANE failure

[David Woodhouse]

From: David Woodhouse <dwmw@xxxxxxxxxxxx>

kvm_create_plane() publishes the new plane in kvm->planes[] and takes
the APICV_INHIBIT_REASON_PLANES inhibit before
kvm_vm_ioctl_create_plane() can still fail at anon_inode_getfile(),
and the error path freed the plane without unpublishing it: a VMM
retrying plane setup after such a failure wrote overlays through the
freed plane via KVM_SET_PLANE_MEMORY_OVERLAY, and VM teardown
destroyed the plane a second time. The VM also kept APICv inhibited
with no nonzero plane left to justify it.

Clear the kvm->planes[] slot before freeing, and drop the APICv
inhibit in kvm_arch_plane_destroy() when the last nonzero plane of a
live VM goes away (at VM teardown plane 0 is already gone, which skips
the update). Make the overlay ioctl read kvm->planes[] under
kvm->lock, which the create path holds across publish and unpublish,
so a plane pointer it observes stays valid until VM teardown; publish
with smp_store_release() for the remaining lock-free readers.

Found by Sashiko review.

Signed-off-by: David Woodhouse <dwmw@xxxxxxxxxxxx>
Assisted-by: LLM
---
 arch/x86/kvm/x86.c  | 32 ++++++++++++++++++++++++++++++++
 virt/kvm/kvm_main.c | 25 ++++++++++++++++++++++---
 2 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c5122e6f42bd..afd668c6db3f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -14003,7 +14003,39 @@ int kvm_arch_plane_init(struct kvm *kvm, struct kvm_plane *plane,
 
 void kvm_arch_plane_destroy(struct kvm_plane *plane)
 {
+	struct kvm *kvm = plane->kvm;
+	int i;
+
 	kvfree(rcu_dereference_check(plane->arch.apic_map, 1));
+
+	if (plane->level == 0)
+		return;
+
+	/*
+	 * Drop the APICv inhibit taken in kvm_arch_plane_init() when the
+	 * last nonzero plane goes away.  Only the KVM_CREATE_PLANE error
+	 * path destroys a nonzero plane while the VM is live; at VM
+	 * teardown plane 0 is destroyed first and kvm->planes[0] is
+	 * already NULL, which skips the update (the inhibit machinery
+	 * needs plane 0 and SRCU, both gone by then).
+	 */
+	if (!kvm->planes[0])
+		return;
+
+	/*
+	 * Reaching here means a nonzero plane died while the VM is live,
+	 * which only the KVM_CREATE_PLANE error path does - under
+	 * kvm->lock.  A future kvm_destroy_planes() reordered to tear
+	 * nonzero planes down before plane 0 would get here locklessly;
+	 * catch that instead of silently poking freed APICv state.
+	 */
+	lockdep_assert_held(&kvm->lock);
+
+	for (i = 1; i < KVM_MAX_PLANES; i++)
+		if (kvm->planes[i] && kvm->planes[i] != plane)
+			return;
+
+	kvm_clear_apicv_inhibit(kvm, APICV_INHIBIT_REASON_PLANES);
 }
 
 int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3e021683c8a8..ae998cef3c36 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1259,7 +1259,12 @@ static struct kvm_plane *kvm_create_plane(struct kvm *kvm, unsigned plane_level)
 	if (kvm_arch_plane_init(kvm, plane, plane_level))
 		goto out_free_plane;
 
-	kvm->planes[plane_level] = plane;
+	/*
+	 * Pairs with the lock-free readers of kvm->planes[] (MSI delivery,
+	 * SEV VMPL targeting): the plane's fields must be visible before
+	 * the pointer is.
+	 */
+	smp_store_release(&kvm->planes[plane_level], plane);
 
 	return plane;
 
@@ -5915,8 +5920,15 @@ static int kvm_vm_ioctl_create_plane(struct kvm *kvm, unsigned id)
 	return fd;
 
 put_kvm:
-	kvm_put_kvm(kvm);
+	/*
+	 * kvm_create_plane() published the plane; unpublish it before
+	 * freeing (kvm->lock is held throughout, so readers who take it
+	 * never observe the dead pointer) and so that VM teardown does
+	 * not destroy it a second time.
+	 */
+	kvm->planes[id] = NULL;
 	kvm_destroy_one_plane(plane);
+	kvm_put_kvm(kvm);
 put_fd:
 	put_unused_fd(fd);
 	return r;
@@ -6037,7 +6049,14 @@ static int kvm_vm_ioctl_set_plane_overlay(struct kvm *kvm,
 		return -EINVAL;
 	if (ov->uaddr & ~PAGE_MASK)
 		return -EINVAL;
-	plane = kvm->planes[ov->plane];
+	/*
+	 * Read under kvm->lock: KVM_CREATE_PLANE publishes and, on its
+	 * error path, unpublishes-and-frees the plane under that lock, so
+	 * a pointer observed here is either NULL or a plane that lives
+	 * until VM teardown.
+	 */
+	scoped_guard(mutex, &kvm->lock)
+		plane = kvm->planes[ov->plane];
 	if (!plane)
 		return -EINVAL;
 
-- 
2.50.1