[PATCH v3 0/6] KVM: Clean up debugfs init/destroy

[PATCH v3 0/6] KVM: Clean up debugfs init/destroy

[Oliver Upton]

From: Oliver Upton <oupton@google.com>

The way that KVM handles debugfs init/destroy is somewhat sloppy. Even
though debugfs is stood up after kvm_create_vm(), it is torn down from
kvm_destroy_vm(). There exists a window where we need to tear down a VM
before debugfs is created, requiring delicate handling.

This series cleans up the debugfs lifecycle by fully tying it to the
VM's init/destroy pattern.

First two patches hoist some unrelated stats initialization to a more
appropriate place for kvm and kvm_vcpu.

The next 3 patches are the meat of the series, changing around the
initialization order to get an FD early and wiring in debugfs
initialization to kvm_create_vm().

Lastly, patch 6 is essentially a revert of Sean's fix [1] for a NULL deref
in debugfs, though I stopped short of an outright revert since that one
went to stable and is still entirely correct.

"Works on my machine", and with luck it will on yours too.

[1] 5c697c367a66 ("KVM: Initialize debugfs_dentry when a VM is created to avoid NULL deref")

v1: http://lore.kernel.org/r/20220415201542.1496582-1-oupton@google.com
v2: https://lore.kernel.org/kvm/20220518175811.2758661-1-oupton@google.com

v1 -> v2:
 - Don't conflate debugfs+stats. Initialize stats_id outside of the
   context of debugfs (Sean)
 - Pass around the FD as a string to avoid subsequent KVM changes
   inappropriately using the FD (Sean)

v2 -> v3:
 - Spare readers from needing to refer to the title of a commit (Sean)
 - Crack fd stringization and move of kvm_create_vm_debugfs() into two
   patches (Sean)
 - Fix a bug that crops up in the middle of the series. Failed to pass
   the fd through to kvm_create_vm_debugfs()

Oliver Upton (6):
  KVM: Shove vm stats_id init into kvm_create_vm()
  KVM: Shove vcpu stats_id init into kvm_vcpu_init()
  KVM: Get an fd before creating the VM
  KVM: Pass the name of the VM fd to kvm_create_vm_debugfs()
  KVM: Actually create debugfs in kvm_create_vm()
  KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

 virt/kvm/kvm_main.c | 91 +++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 41 deletions(-)


base-commit: 8031d87aa9953ddeb047a5356ebd0b240c30f233
-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v3 1/6] KVM: Shove vm stats_id init into kvm_create_vm()

[Oliver Upton]

From: Oliver Upton <oupton@google.com>

Initialize stats_id alongside the other struct kvm fields to futureproof
against possible initialization order mistakes in KVM. While at it, move
the format string to the first line of the call and fix the indentation
of the second line.

No functional change intended.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 virt/kvm/kvm_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index da263c370d00..cc760ebcd390 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1155,6 +1155,9 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	 */
 	kvm->debugfs_dentry = ERR_PTR(-ENOENT);
 
+	snprintf(kvm->stats_id, sizeof(kvm->stats_id), "kvm-%d",
+		 task_pid_nr(current));
+
 	if (init_srcu_struct(&kvm->srcu))
 		goto out_err_no_srcu;
 	if (init_srcu_struct(&kvm->irq_srcu))
@@ -4902,9 +4905,6 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	if (r < 0)
 		goto put_kvm;
 
-	snprintf(kvm->stats_id, sizeof(kvm->stats_id),
-			"kvm-%d", task_pid_nr(current));
-
 	file = anon_inode_getfile("kvm-vm", &kvm_vm_fops, kvm, O_RDWR);
 	if (IS_ERR(file)) {
 		put_unused_fd(r);
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v3 1/6] KVM: Shove vm stats_id init into kvm_create_vm()

[Sean Christopherson]

On Wed, Jul 20, 2022, Oliver Upton wrote:
> From: Oliver Upton <oupton@google.com>
> 
> Initialize stats_id alongside the other struct kvm fields to futureproof
> against possible initialization order mistakes in KVM.

Same nit about handwaving.  Maybe it's just me, but even though I already know
what this patch is doing, this changelog still somehow leaves me wondering what
"possible initialization order mistakes" this prevents.

> While at it, move the format string to the first line of the call and fix the
> indentation of the second line.
> 
> No functional change intended.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[PATCH v3 2/6] KVM: Shove vcpu stats_id init into kvm_vcpu_init()

[Oliver Upton]

From: Oliver Upton <oupton@google.com>

Initialize stats_id alongside other kvm_vcpu fields to futureproof
against possible initialization order mistakes in KVM.

No functional change intended.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 virt/kvm/kvm_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cc760ebcd390..1f78b7ad5430 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -484,6 +484,10 @@ static void kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	vcpu->ready = false;
 	preempt_notifier_init(&vcpu->preempt_notifier, &kvm_preempt_ops);
 	vcpu->last_used_slot = NULL;
+
+	/* Fill the stats id string for the vcpu */
+	snprintf(vcpu->stats_id, sizeof(vcpu->stats_id), "kvm-%d/vcpu-%d",
+		 task_pid_nr(current), id);
 }
 
 static void kvm_vcpu_destroy(struct kvm_vcpu *vcpu)
@@ -3919,10 +3923,6 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
 	if (r)
 		goto unlock_vcpu_destroy;
 
-	/* Fill the stats id string for the vcpu */
-	snprintf(vcpu->stats_id, sizeof(vcpu->stats_id), "kvm-%d/vcpu-%d",
-		 task_pid_nr(current), id);
-
 	/* Now it's all set up, let userspace reach it */
 	kvm_get_kvm(kvm);
 	r = create_vcpu_fd(vcpu);
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v3 2/6] KVM: Shove vcpu stats_id init into kvm_vcpu_init()

[Sean Christopherson]

On Wed, Jul 20, 2022, Oliver Upton wrote:
> From: Oliver Upton <oupton@google.com>
> 
> Initialize stats_id alongside other kvm_vcpu fields to futureproof
> against possible initialization order mistakes in KVM.

Nit, I dislike the handwaving, it doesn't take much effort to explain exactly
what this guards against.

  Initialize stats_id alongside other kvm_vcpu fields to make it more
  difficult to unintentionally access stats_id before it's set.

> No functional change intended.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v3 2/6] KVM: Shove vcpu stats_id init into kvm_vcpu_init()

[Paolo Bonzini]

On 8/5/22 21:06, Sean Christopherson wrote:
>    Initialize stats_id alongside other kvm_vcpu fields to make it more
>    difficult to unintentionally access stats_id before it's set.

Thanks for the proposal, I'll fix it when applying.

Paolo

[PATCH v3 3/6] KVM: Get an fd before creating the VM

[Oliver Upton]

From: Oliver Upton <oupton@google.com>

Allocate a VM's fd at the very beginning of kvm_dev_ioctl_create_vm() so
that KVM can use the fd value to generate strigns, e.g. for debugfs,
when creating and initializing the VM.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 virt/kvm/kvm_main.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1f78b7ad5430..e270cff3c9f4 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4889,25 +4889,27 @@ EXPORT_SYMBOL_GPL(file_is_kvm);
 
 static int kvm_dev_ioctl_create_vm(unsigned long type)
 {
-	int r;
+	int r, fd;
 	struct kvm *kvm;
 	struct file *file;
 
+	fd = get_unused_fd_flags(O_CLOEXEC);
+	if (fd < 0)
+		return fd;
+
 	kvm = kvm_create_vm(type);
-	if (IS_ERR(kvm))
-		return PTR_ERR(kvm);
+	if (IS_ERR(kvm)) {
+		r = PTR_ERR(kvm);
+		goto put_fd;
+	}
+
 #ifdef CONFIG_KVM_MMIO
 	r = kvm_coalesced_mmio_init(kvm);
 	if (r < 0)
 		goto put_kvm;
 #endif
-	r = get_unused_fd_flags(O_CLOEXEC);
-	if (r < 0)
-		goto put_kvm;
-
 	file = anon_inode_getfile("kvm-vm", &kvm_vm_fops, kvm, O_RDWR);
 	if (IS_ERR(file)) {
-		put_unused_fd(r);
 		r = PTR_ERR(file);
 		goto put_kvm;
 	}
@@ -4918,18 +4920,20 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	 * cases it will be called by the final fput(file) and will take
 	 * care of doing kvm_put_kvm(kvm).
 	 */
-	if (kvm_create_vm_debugfs(kvm, r) < 0) {
-		put_unused_fd(r);
+	if (kvm_create_vm_debugfs(kvm, fd) < 0) {
 		fput(file);
-		return -ENOMEM;
+		r = -ENOMEM;
+		goto put_fd;
 	}
 	kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
 
-	fd_install(r, file);
-	return r;
+	fd_install(fd, file);
+	return fd;
 
 put_kvm:
 	kvm_put_kvm(kvm);
+put_fd:
+	put_unused_fd(fd);
 	return r;
 }
 
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v3 3/6] KVM: Get an fd before creating the VM

[Sean Christopherson]

On Wed, Jul 20, 2022, Oliver Upton wrote:
> From: Oliver Upton <oupton@google.com>
> 
> Allocate a VM's fd at the very beginning of kvm_dev_ioctl_create_vm() so
> that KVM can use the fd value to generate strigns, e.g. for debugfs,

s/strigns/strings

> when creating and initializing the VM.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---

Typo aside,

Reviewed-by: Sean Christopherson <seanjc@google.com>

[PATCH v3 4/6] KVM: Pass the name of the VM fd to kvm_create_vm_debugfs()

[Oliver Upton]

From: Oliver Upton <oupton@google.com>

At the time the VM fd is used in kvm_create_vm_debugfs(), the fd has
been allocated but not yet installed. It is only really useful as an
identifier in strings for the VM (such as debugfs).

Treat it exactly as such by passing the string name of the fd to
kvm_create_vm_debugfs(), futureproofing against possible misuse of the
VM fd.

Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Oliver Upton <oupton@google.com>
---
 virt/kvm/kvm_main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e270cff3c9f4..1e7f780a357b 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1021,7 +1021,7 @@ static void kvm_destroy_vm_debugfs(struct kvm *kvm)
 	}
 }
 
-static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
+static int kvm_create_vm_debugfs(struct kvm *kvm, const char *fdname)
 {
 	static DEFINE_MUTEX(kvm_debugfs_lock);
 	struct dentry *dent;
@@ -1035,7 +1035,7 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 	if (!debugfs_initialized())
 		return 0;
 
-	snprintf(dir_name, sizeof(dir_name), "%d-%d", task_pid_nr(current), fd);
+	snprintf(dir_name, sizeof(dir_name), "%d-%s", task_pid_nr(current), fdname);
 	mutex_lock(&kvm_debugfs_lock);
 	dent = debugfs_lookup(dir_name, kvm_debugfs_dir);
 	if (dent) {
@@ -4889,6 +4889,7 @@ EXPORT_SYMBOL_GPL(file_is_kvm);
 
 static int kvm_dev_ioctl_create_vm(unsigned long type)
 {
+	char fdname[ITOA_MAX_LEN + 1];
 	int r, fd;
 	struct kvm *kvm;
 	struct file *file;
@@ -4897,6 +4898,8 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	if (fd < 0)
 		return fd;
 
+	snprintf(fdname, sizeof(fdname), "%d", fd);
+
 	kvm = kvm_create_vm(type);
 	if (IS_ERR(kvm)) {
 		r = PTR_ERR(kvm);
@@ -4920,7 +4923,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	 * cases it will be called by the final fput(file) and will take
 	 * care of doing kvm_put_kvm(kvm).
 	 */
-	if (kvm_create_vm_debugfs(kvm, fd) < 0) {
+	if (kvm_create_vm_debugfs(kvm, fdname) < 0) {
 		fput(file);
 		r = -ENOMEM;
 		goto put_fd;
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v3 4/6] KVM: Pass the name of the VM fd to kvm_create_vm_debugfs()

[Sean Christopherson]

On Wed, Jul 20, 2022, Oliver Upton wrote:
> From: Oliver Upton <oupton@google.com>
> 
> At the time the VM fd is used in kvm_create_vm_debugfs(), the fd has
> been allocated but not yet installed. It is only really useful as an
> identifier in strings for the VM (such as debugfs).
> 
> Treat it exactly as such by passing the string name of the fd to
> kvm_create_vm_debugfs(), futureproofing against possible misuse of the
> VM fd.

One last whine session,

  kvm_create_vm_debugfs() to guard against attempts to consume the fd
  before it is installed.

> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[PATCH v3 5/6] KVM: Actually create debugfs in kvm_create_vm()

[Oliver Upton]

From: Oliver Upton <oupton@google.com>

Doing debugfs creation after vm creation leaves things in a
quasi-initialized state for a while. This is further complicated by the
fact that we tear down debugfs from kvm_destroy_vm(). Align debugfs and
stats init/destroy with the vm init/destroy pattern to avoid any
headaches.

Note the fix for a benign mistake in error handling for calls to
kvm_arch_create_vm_debugfs() rolled in. Since all implementations of
the function return 0 unconditionally it isn't actually a bug at
the moment.

Lastly, tear down debugfs/stats data in the kvm_create_vm_debugfs()
error path. Previously it was safe to assume that kvm_destroy_vm() would
take out the garbage, that is no longer the case.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 virt/kvm/kvm_main.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1e7f780a357b..609f49a133f8 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1028,7 +1028,7 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, const char *fdname)
 	char dir_name[ITOA_MAX_LEN * 2];
 	struct kvm_stat_data *stat_data;
 	const struct _kvm_stats_desc *pdesc;
-	int i, ret;
+	int i, ret = -ENOMEM;
 	int kvm_debugfs_num_entries = kvm_vm_stats_header.num_desc +
 				      kvm_vcpu_stats_header.num_desc;
 
@@ -1054,13 +1054,13 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, const char *fdname)
 					 sizeof(*kvm->debugfs_stat_data),
 					 GFP_KERNEL_ACCOUNT);
 	if (!kvm->debugfs_stat_data)
-		return -ENOMEM;
+		goto out_err;
 
 	for (i = 0; i < kvm_vm_stats_header.num_desc; ++i) {
 		pdesc = &kvm_vm_stats_desc[i];
 		stat_data = kzalloc(sizeof(*stat_data), GFP_KERNEL_ACCOUNT);
 		if (!stat_data)
-			return -ENOMEM;
+			goto out_err;
 
 		stat_data->kvm = kvm;
 		stat_data->desc = pdesc;
@@ -1075,7 +1075,7 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, const char *fdname)
 		pdesc = &kvm_vcpu_stats_desc[i];
 		stat_data = kzalloc(sizeof(*stat_data), GFP_KERNEL_ACCOUNT);
 		if (!stat_data)
-			return -ENOMEM;
+			goto out_err;
 
 		stat_data->kvm = kvm;
 		stat_data->desc = pdesc;
@@ -1087,12 +1087,13 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, const char *fdname)
 	}
 
 	ret = kvm_arch_create_vm_debugfs(kvm);
-	if (ret) {
-		kvm_destroy_vm_debugfs(kvm);
-		return i;
-	}
+	if (ret)
+		goto out_err;
 
 	return 0;
+out_err:
+	kvm_destroy_vm_debugfs(kvm);
+	return ret;
 }
 
 /*
@@ -1123,7 +1124,7 @@ int __weak kvm_arch_create_vm_debugfs(struct kvm *kvm)
 	return 0;
 }
 
-static struct kvm *kvm_create_vm(unsigned long type)
+static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
 {
 	struct kvm *kvm = kvm_arch_alloc_vm();
 	struct kvm_memslots *slots;
@@ -1212,7 +1213,7 @@ static struct kvm *kvm_create_vm(unsigned long type)
 
 	r = kvm_arch_post_init_vm(kvm);
 	if (r)
-		goto out_err;
+		goto out_err_mmu_notifier;
 
 	mutex_lock(&kvm_lock);
 	list_add(&kvm->vm_list, &vm_list);
@@ -1228,12 +1229,18 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	 */
 	if (!try_module_get(kvm_chardev_ops.owner)) {
 		r = -ENODEV;
-		goto out_err;
+		goto out_err_mmu_notifier;
 	}
 
+	r = kvm_create_vm_debugfs(kvm, fdname);
+	if (r)
+		goto out_err;
+
 	return kvm;
 
 out_err:
+	module_put(kvm_chardev_ops.owner);
+out_err_mmu_notifier:
 #if defined(CONFIG_MMU_NOTIFIER) && defined(KVM_ARCH_WANT_MMU_NOTIFIER)
 	if (kvm->mmu_notifier.ops)
 		mmu_notifier_unregister(&kvm->mmu_notifier, current->mm);
@@ -4900,7 +4907,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 
 	snprintf(fdname, sizeof(fdname), "%d", fd);
 
-	kvm = kvm_create_vm(type);
+	kvm = kvm_create_vm(type, fdname);
 	if (IS_ERR(kvm)) {
 		r = PTR_ERR(kvm);
 		goto put_fd;
@@ -4923,11 +4930,6 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	 * cases it will be called by the final fput(file) and will take
 	 * care of doing kvm_put_kvm(kvm).
 	 */
-	if (kvm_create_vm_debugfs(kvm, fdname) < 0) {
-		fput(file);
-		r = -ENOMEM;
-		goto put_fd;
-	}
 	kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
 
 	fd_install(fd, file);
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v3 5/6] KVM: Actually create debugfs in kvm_create_vm()

[Sean Christopherson]

On Wed, Jul 20, 2022, Oliver Upton wrote:
> From: Oliver Upton <oupton@google.com>
> 
> Doing debugfs creation after vm creation leaves things in a
> quasi-initialized state for a while. This is further complicated by the
> fact that we tear down debugfs from kvm_destroy_vm(). Align debugfs and
> stats init/destroy with the vm init/destroy pattern to avoid any
> headaches.
> 
> Note the fix for a benign mistake in error handling for calls to
> kvm_arch_create_vm_debugfs() rolled in. Since all implementations of
> the function return 0 unconditionally it isn't actually a bug at
> the moment.

Heh, now I feel like you're being intentionally mean.  Usually I discourage
play-by-play descriptions, but in this case being explicit saves readers a lot
of staring.

And I would argue it's still a bug, it's just that the bug can't be hit and so
can't cause issues.  E.g.

  Opportunistically fix a benign bug where KVM would return "i" instead
  of "ret" if kvm_arch_create_vm_debugfs() failed.  The bug is benign as
  all implementations of the function return 0 unconditionally.

> Lastly, tear down debugfs/stats data in the kvm_create_vm_debugfs()
> error path. Previously it was safe to assume that kvm_destroy_vm() would
> take out the garbage, that is no longer the case.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

[PATCH v3 6/6] KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

[Oliver Upton]

From: Oliver Upton <oupton@google.com>

Since KVM now sanely handles debugfs init/destroy w.r.t. the VM, it is
safe to hoist kvm_create_vm_debugfs() back into kvm_create_vm(). The
author of this commit remains bitter for having been burned by the old
wreck in commit a44a4cc1c969 ("KVM: Don't create VM debugfs files
outside of the VM directory").

Signed-off-by: Oliver Upton <oupton@google.com>
Reviewed-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 609f49a133f8..7ac60f75cfa1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1032,6 +1032,12 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, const char *fdname)
 	int kvm_debugfs_num_entries = kvm_vm_stats_header.num_desc +
 				      kvm_vcpu_stats_header.num_desc;
 
+	/*
+	 * Force subsequent debugfs file creations to fail if the VM directory
+	 * is not created.
+	 */
+	kvm->debugfs_dentry = ERR_PTR(-ENOENT);
+
 	if (!debugfs_initialized())
 		return 0;
 
@@ -1154,12 +1160,6 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
 
 	BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
 
-	/*
-	 * Force subsequent debugfs file creations to fail if the VM directory
-	 * is not created (by kvm_create_vm_debugfs()).
-	 */
-	kvm->debugfs_dentry = ERR_PTR(-ENOENT);
-
 	snprintf(kvm->stats_id, sizeof(kvm->stats_id), "kvm-%d",
 		 task_pid_nr(current));
 
-- 
2.37.0.170.g444d1eabd0-goog

Re: [PATCH v3 6/6] KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

[Sean Christopherson]

On Wed, Jul 20, 2022, Oliver Upton wrote:
> From: Oliver Upton <oupton@google.com>
> 
> Since KVM now sanely handles debugfs init/destroy w.r.t. the VM, it is
> safe to hoist kvm_create_vm_debugfs() back into kvm_create_vm(). The
> author of this commit remains bitter for having been burned by the old
> wreck in commit a44a4cc1c969 ("KVM: Don't create VM debugfs files
> outside of the VM directory").
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> Reviewed-by: Sean Christopherson <seanjc@google.com>

Heh, so this amusingly has my review, but I'd rather omit this patch and leave
the initialization with the pile of other code that initializes fields for which
zero-initialization is insufficient/incorrect.

Any objections to dropping this?

> ---
>  virt/kvm/kvm_main.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 609f49a133f8..7ac60f75cfa1 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1032,6 +1032,12 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, const char *fdname)
>  	int kvm_debugfs_num_entries = kvm_vm_stats_header.num_desc +
>  				      kvm_vcpu_stats_header.num_desc;
>  
> +	/*
> +	 * Force subsequent debugfs file creations to fail if the VM directory
> +	 * is not created.
> +	 */
> +	kvm->debugfs_dentry = ERR_PTR(-ENOENT);
> +
>  	if (!debugfs_initialized())
>  		return 0;
>  
> @@ -1154,12 +1160,6 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
>  
>  	BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
>  
> -	/*
> -	 * Force subsequent debugfs file creations to fail if the VM directory
> -	 * is not created (by kvm_create_vm_debugfs()).
> -	 */
> -	kvm->debugfs_dentry = ERR_PTR(-ENOENT);
> -
>  	snprintf(kvm->stats_id, sizeof(kvm->stats_id), "kvm-%d",
>  		 task_pid_nr(current));
>  
> -- 
> 2.37.0.170.g444d1eabd0-goog
> 

Re: [PATCH v3 6/6] KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

[Paolo Bonzini]

On 8/5/22 21:02, Sean Christopherson wrote:
> Heh, so this amusingly has my review, but I'd rather omit this patch and leave
> the initialization with the pile of other code that initializes fields for which
> zero-initialization is insufficient/incorrect.
> 
> Any objections to dropping this?

Yeah, I was going to say the same.  The points before and after this 
patch are far enough that I'm a bit more confident leaving it out.

Paolo

Re: [PATCH v3 6/6] KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

[Oliver Upton]

On Tue, Aug 09, 2022 at 04:56:28PM +0200, Paolo Bonzini wrote:
> On 8/5/22 21:02, Sean Christopherson wrote:
> > Heh, so this amusingly has my review, but I'd rather omit this patch and leave
> > the initialization with the pile of other code that initializes fields for which
> > zero-initialization is insufficient/incorrect.
> > 
> > Any objections to dropping this?
> 
> Yeah, I was going to say the same.  The points before and after this patch
> are far enough that I'm a bit more confident leaving it out.

Sounds reasonable to me. To be fair, I mostly threw this patch at the
end to poke fun at the original mistake :)

--
Thanks,
Oliver

[PATCH v3 0/6] KVM: Clean up debugfs init/destroy

[Oliver Upton]

From: Oliver Upton <oupton@google.com>

The way that KVM handles debugfs init/destroy is somewhat sloppy. Even
though debugfs is stood up after kvm_create_vm(), it is torn down from
kvm_destroy_vm(). There exists a window where we need to tear down a VM
before debugfs is created, requiring delicate handling.

This series cleans up the debugfs lifecycle by fully tying it to the
VM's init/destroy pattern.

First two patches hoist some unrelated stats initialization to a more
appropriate place for kvm and kvm_vcpu.

The next 3 patches are the meat of the series, changing around the
initialization order to get an FD early and wiring in debugfs
initialization to kvm_create_vm().

Lastly, patch 6 is essentially a revert of Sean's fix [1] for a NULL deref
in debugfs, though I stopped short of an outright revert since that one
went to stable and is still entirely correct.

"Works on my machine", and with luck it will on yours too.

[1] 5c697c367a66 ("KVM: Initialize debugfs_dentry when a VM is created to avoid NULL deref")

v1: http://lore.kernel.org/r/20220415201542.1496582-1-oupton@google.com
v2: https://lore.kernel.org/kvm/20220518175811.2758661-1-oupton@google.com

v1 -> v2:
 - Don't conflate debugfs+stats. Initialize stats_id outside of the
   context of debugfs (Sean)
 - Pass around the FD as a string to avoid subsequent KVM changes
   inappropriately using the FD (Sean)

v2 -> v3:
 - Spare readers from needing to refer to the title of a commit (Sean)
 - Crack fd stringization and move of kvm_create_vm_debugfs() into two
   patches (Sean)
 - Fix a bug that crops up in the middle of the series. Failed to pass
   the fd through to kvm_create_vm_debugfs()

Oliver Upton (6):
  KVM: Shove vm stats_id init into kvm_create_vm()
  KVM: Shove vcpu stats_id init into kvm_vcpu_init()
  KVM: Get an fd before creating the VM
  KVM: Pass the name of the VM fd to kvm_create_vm_debugfs()
  KVM: Actually create debugfs in kvm_create_vm()
  KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

 virt/kvm/kvm_main.c | 91 +++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 41 deletions(-)


base-commit: 8031d87aa9953ddeb047a5356ebd0b240c30f233
-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v3 1/6] KVM: Shove vm stats_id init into kvm_create_vm()

[Oliver Upton]

From: Oliver Upton <oupton@google.com>

Initialize stats_id alongside the other struct kvm fields to futureproof
against possible initialization order mistakes in KVM. While at it, move
the format string to the first line of the call and fix the indentation
of the second line.

No functional change intended.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 virt/kvm/kvm_main.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index da263c370d00..cc760ebcd390 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1155,6 +1155,9 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	 */
 	kvm->debugfs_dentry = ERR_PTR(-ENOENT);
 
+	snprintf(kvm->stats_id, sizeof(kvm->stats_id), "kvm-%d",
+		 task_pid_nr(current));
+
 	if (init_srcu_struct(&kvm->srcu))
 		goto out_err_no_srcu;
 	if (init_srcu_struct(&kvm->irq_srcu))
@@ -4902,9 +4905,6 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	if (r < 0)
 		goto put_kvm;
 
-	snprintf(kvm->stats_id, sizeof(kvm->stats_id),
-			"kvm-%d", task_pid_nr(current));
-
 	file = anon_inode_getfile("kvm-vm", &kvm_vm_fops, kvm, O_RDWR);
 	if (IS_ERR(file)) {
 		put_unused_fd(r);
-- 
2.37.0.170.g444d1eabd0-goog

[PATCH v3 2/6] KVM: Shove vcpu stats_id init into kvm_vcpu_init()

[Oliver Upton]

From: Oliver Upton <oupton@google.com>

Initialize stats_id alongside other kvm_vcpu fields to futureproof
against possible initialization order mistakes in KVM.

No functional change intended.

Signed-off-by: Oliver Upton <oupton@google.com>
---
 virt/kvm/kvm_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cc760ebcd390..1f78b7ad5430 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -484,6 +484,10 @@ static void kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)
 	vcpu->ready = false;
 	preempt_notifier_init(&vcpu->preempt_notifier, &kvm_preempt_ops);
 	vcpu->last_used_slot = NULL;
+
+	/* Fill the stats id string for the vcpu */
+	snprintf(vcpu->stats_id, sizeof(vcpu->stats_id), "kvm-%d/vcpu-%d",
+		 task_pid_nr(current), id);
 }
 
 static void kvm_vcpu_destroy(struct kvm_vcpu *vcpu)
@@ -3919,10 +3923,6 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)
 	if (r)
 		goto unlock_vcpu_destroy;
 
-	/* Fill the stats id string for the vcpu */
-	snprintf(vcpu->stats_id, sizeof(vcpu->stats_id), "kvm-%d/vcpu-%d",
-		 task_pid_nr(current), id);
-
 	/* Now it's all set up, let userspace reach it */
 	kvm_get_kvm(kvm);
 	r = create_vcpu_fd(vcpu);
-- 
2.37.0.170.g444d1eabd0-goog