[PATCH v2 0/5] KVM: Clean up debugfs+stats init/destroy

[PATCH v2 0/5] KVM: Clean up debugfs+stats init/destroy

[Oliver Upton]

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.

Second two 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 5 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.

Applies cleanly to v5.18-rc5, since [1] is currently missing from
kvm/queue or kvm/next. Tested with KVM selftests and the reproducer
mentioned in [1] on an Intel Skylake machine.

[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

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)

Oliver Upton (5):
  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: Actually create debugfs in kvm_create_vm()
  KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

 virt/kvm/kvm_main.c | 96 +++++++++++++++++++++++----------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

-- 
2.36.1.124.g0e6072fb45-goog

[PATCH v2 0/5] KVM: Clean up debugfs+stats init/destroy

[Oliver Upton]

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.

Second two 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 5 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.

Applies cleanly to v5.18-rc5, since [1] is currently missing from
kvm/queue or kvm/next. Tested with KVM selftests and the reproducer
mentioned in [1] on an Intel Skylake machine.

[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

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)

Oliver Upton (5):
  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: Actually create debugfs in kvm_create_vm()
  KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

 virt/kvm/kvm_main.c | 96 +++++++++++++++++++++++----------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

-- 
2.36.1.124.g0e6072fb45-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 1/5] KVM: Shove vm stats_id init into kvm_create_vm()

[Oliver Upton]

Initialize the field alongside the other struct kvm fields. 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 6d971fb1b08d..36dc9271d039 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1101,6 +1101,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))
@@ -4787,9 +4790,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.36.1.124.g0e6072fb45-goog

[PATCH v2 1/5] KVM: Shove vm stats_id init into kvm_create_vm()

[Oliver Upton]

Initialize the field alongside the other struct kvm fields. 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 6d971fb1b08d..36dc9271d039 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1101,6 +1101,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))
@@ -4787,9 +4790,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.36.1.124.g0e6072fb45-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 2/5] KVM: Shove vcpu stats_id init into kvm_vcpu_init()

[Oliver Upton]

Initialize the field alongside other kvm_vcpu fields. 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 36dc9271d039..778151333ac0 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -440,6 +440,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)
@@ -3807,10 +3811,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.36.1.124.g0e6072fb45-goog

[PATCH v2 2/5] KVM: Shove vcpu stats_id init into kvm_vcpu_init()

[Oliver Upton]

Initialize the field alongside other kvm_vcpu fields. 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 36dc9271d039..778151333ac0 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -440,6 +440,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)
@@ -3807,10 +3811,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.36.1.124.g0e6072fb45-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 3/5] KVM: Get an fd before creating the VM

[Oliver Upton]

Hoist fd init to the very beginning of kvm_create_vm() so we can make
use of it in other init routines.

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

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 778151333ac0..87ccab74dc80 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4774,25 +4774,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;
 	}
@@ -4804,17 +4806,19 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	 * care of doing kvm_put_kvm(kvm).
 	 */
 	if (kvm_create_vm_debugfs(kvm, r) < 0) {
-		put_unused_fd(r);
 		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.36.1.124.g0e6072fb45-goog

[PATCH v2 3/5] KVM: Get an fd before creating the VM

[Oliver Upton]

Hoist fd init to the very beginning of kvm_create_vm() so we can make
use of it in other init routines.

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

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 778151333ac0..87ccab74dc80 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4774,25 +4774,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;
 	}
@@ -4804,17 +4806,19 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	 * care of doing kvm_put_kvm(kvm).
 	 */
 	if (kvm_create_vm_debugfs(kvm, r) < 0) {
-		put_unused_fd(r);
 		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.36.1.124.g0e6072fb45-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 4/5] KVM: Actually create debugfs in kvm_create_vm()

[Oliver Upton]

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. Pass around the fd number as a string, as poking at the fd in
any other way is nonsensical.

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 | 48 ++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 87ccab74dc80..aaa7213b34dd 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -968,21 +968,21 @@ 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;
 	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;
 
 	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) {
@@ -1001,13 +1001,13 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 					 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;
@@ -1022,7 +1022,7 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 		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;
@@ -1034,12 +1034,13 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 	}
 
 	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;
 }
 
 /*
@@ -1070,7 +1071,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;
@@ -1158,7 +1159,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);
@@ -1174,12 +1175,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);
@@ -4774,6 +4781,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;
@@ -4782,7 +4790,8 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	if (fd < 0)
 		return fd;
 
-	kvm = kvm_create_vm(type);
+	snprintf(fdname, sizeof(fdname), "%d", fd);
+	kvm = kvm_create_vm(type, fdname);
 	if (IS_ERR(kvm)) {
 		r = PTR_ERR(kvm);
 		goto put_fd;
@@ -4799,17 +4808,6 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 		goto put_kvm;
 	}
 
-	/*
-	 * Don't call kvm_put_kvm anymore at this point; file->f_op is
-	 * already set, with ->release() being kvm_vm_release().  In error
-	 * 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) {
-		fput(file);
-		r = -ENOMEM;
-		goto put_fd;
-	}
 	kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
 
 	fd_install(fd, file);
-- 
2.36.1.124.g0e6072fb45-goog

[PATCH v2 4/5] KVM: Actually create debugfs in kvm_create_vm()

[Oliver Upton]

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. Pass around the fd number as a string, as poking at the fd in
any other way is nonsensical.

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 | 48 ++++++++++++++++++++++-----------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 87ccab74dc80..aaa7213b34dd 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -968,21 +968,21 @@ 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;
 	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;
 
 	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) {
@@ -1001,13 +1001,13 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 					 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;
@@ -1022,7 +1022,7 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 		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;
@@ -1034,12 +1034,13 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
 	}
 
 	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;
 }
 
 /*
@@ -1070,7 +1071,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;
@@ -1158,7 +1159,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);
@@ -1174,12 +1175,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);
@@ -4774,6 +4781,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;
@@ -4782,7 +4790,8 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 	if (fd < 0)
 		return fd;
 
-	kvm = kvm_create_vm(type);
+	snprintf(fdname, sizeof(fdname), "%d", fd);
+	kvm = kvm_create_vm(type, fdname);
 	if (IS_ERR(kvm)) {
 		r = PTR_ERR(kvm);
 		goto put_fd;
@@ -4799,17 +4808,6 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
 		goto put_kvm;
 	}
 
-	/*
-	 * Don't call kvm_put_kvm anymore at this point; file->f_op is
-	 * already set, with ->release() being kvm_vm_release().  In error
-	 * 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) {
-		fput(file);
-		r = -ENOMEM;
-		goto put_fd;
-	}
 	kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
 
 	fd_install(fd, file);
-- 
2.36.1.124.g0e6072fb45-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v2 5/5] KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

[Oliver Upton]

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>
---
 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 aaa7213b34dd..558de6a252de 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -979,6 +979,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;
 
@@ -1100,12 +1106,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.36.1.124.g0e6072fb45-goog

[PATCH v2 5/5] KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

[Oliver Upton]

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>
---
 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 aaa7213b34dd..558de6a252de 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -979,6 +979,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;
 
@@ -1100,12 +1106,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.36.1.124.g0e6072fb45-goog

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> Initialize the field alongside the other struct kvm fields. No

Restate "stats_id" instead of "the field", it's literally fewer characters and
having to go read subject/shortlog to grok the change is annoying.  IMO, changelogs
should be 100% coherent without the shortlog.

Explaining why would also be helpful.  AFAICT there's no actual "need" for this
in this series, rather that this is futureproofing KVM since there's no reason
not to fill kvm->stats_id from time zero.

> 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 6d971fb1b08d..36dc9271d039 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1101,6 +1101,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))
> @@ -4787,9 +4790,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.36.1.124.g0e6072fb45-goog
> 

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

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> Initialize the field alongside the other struct kvm fields. No

Restate "stats_id" instead of "the field", it's literally fewer characters and
having to go read subject/shortlog to grok the change is annoying.  IMO, changelogs
should be 100% coherent without the shortlog.

Explaining why would also be helpful.  AFAICT there's no actual "need" for this
in this series, rather that this is futureproofing KVM since there's no reason
not to fill kvm->stats_id from time zero.

> 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 6d971fb1b08d..36dc9271d039 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1101,6 +1101,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))
> @@ -4787,9 +4790,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.36.1.124.g0e6072fb45-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> Initialize the field alongside other kvm_vcpu fields. No functional
> change intended.

Same complaints as the previous changelog.

> 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 36dc9271d039..778151333ac0 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -440,6 +440,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)
> @@ -3807,10 +3811,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.36.1.124.g0e6072fb45-goog
> 

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

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> Initialize the field alongside other kvm_vcpu fields. No functional
> change intended.

Same complaints as the previous changelog.

> 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 36dc9271d039..778151333ac0 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -440,6 +440,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)
> @@ -3807,10 +3811,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.36.1.124.g0e6072fb45-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> Initialize the field alongside the other struct kvm fields. 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 6d971fb1b08d..36dc9271d039 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1101,6 +1101,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));

After looking at the next patch, can you opportunistically tweak this to (a) have
the string on the first line, and (b) align indentation?  I.e.

	snprintf(kvm->stats_id, sizeof(kvm->stats_id), "kvm-%d",
		 task_pid_nr(current));

That makes it a lot easier to see what the string will look like.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> Initialize the field alongside the other struct kvm fields. 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 6d971fb1b08d..36dc9271d039 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1101,6 +1101,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));

After looking at the next patch, can you opportunistically tweak this to (a) have
the string on the first line, and (b) align indentation?  I.e.

	snprintf(kvm->stats_id, sizeof(kvm->stats_id), "kvm-%d",
		 task_pid_nr(current));

That makes it a lot easier to see what the string will look like.

Re: [PATCH v2 3/5] KVM: Get an fd before creating the VM

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> Hoist fd init to the very beginning of kvm_create_vm() so we can make

"init" is a bit ambiguous, e.g. from a "can I use the fd" perspective the fd is
still very much not initialized.  Also, it's at the beginning of
kvm_dev_ioctl_create_vm(), _before_ kvm_create_vm().

  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 strings, e.g. for debugfs,
  when creating and initializing the VM.

> use of it in other init routines.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---

Re: [PATCH v2 3/5] KVM: Get an fd before creating the VM

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> Hoist fd init to the very beginning of kvm_create_vm() so we can make

"init" is a bit ambiguous, e.g. from a "can I use the fd" perspective the fd is
still very much not initialized.  Also, it's at the beginning of
kvm_dev_ioctl_create_vm(), _before_ kvm_create_vm().

  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 strings, e.g. for debugfs,
  when creating and initializing the VM.

> use of it in other init routines.
> 
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 4/5] KVM: Actually create debugfs in kvm_create_vm()

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> 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. Pass around the fd number as a string, as poking at the fd in
> any other way is nonsensical.

"any other way before it is installed", otherwise it sounds like the fd is this
magic black box that KVM can't touch.

And the changes to pass @fdname instead of @fd should be a separate patch, both to
reduce churn and because it's not a risk free change, e.g. if this is the improper
size then bisection should point at the fdname patch, not at this combined patch.

	char fdname[ITOA_MAX_LEN + 1];

> 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>
> ---

...

> @@ -4774,6 +4781,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;
> @@ -4782,7 +4790,8 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
>  	if (fd < 0)
>  		return fd;
>  
> -	kvm = kvm_create_vm(type);
> +	snprintf(fdname, sizeof(fdname), "%d", fd);

Nit, I'd prefer a blank line here so that it's easier to see the call to
kvm_create_vm().

> +	kvm = kvm_create_vm(type, fdname);
>  	if (IS_ERR(kvm)) {
>  		r = PTR_ERR(kvm);
>  		goto put_fd;
> @@ -4799,17 +4808,6 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
>  		goto put_kvm;
>  	}
>  
> -	/*
> -	 * Don't call kvm_put_kvm anymore at this point; file->f_op is
> -	 * already set, with ->release() being kvm_vm_release().  In error
> -	 * cases it will be called by the final fput(file) and will take
> -	 * care of doing kvm_put_kvm(kvm).
> -	 */

I think we should keep the comment to warn future developers.  I'm tempted to say
it could be reworded to say something like "if you're adding a call that can fail
at this point, you're doing it wrong".  But for this patch, I'd say just leave the
comment intact.

> -	if (kvm_create_vm_debugfs(kvm, r) < 0) {
> -		fput(file);
> -		r = -ENOMEM;
> -		goto put_fd;
> -	}
>  	kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
>  
>  	fd_install(fd, file);
> -- 
> 2.36.1.124.g0e6072fb45-goog
> 

Re: [PATCH v2 4/5] KVM: Actually create debugfs in kvm_create_vm()

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> 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. Pass around the fd number as a string, as poking at the fd in
> any other way is nonsensical.

"any other way before it is installed", otherwise it sounds like the fd is this
magic black box that KVM can't touch.

And the changes to pass @fdname instead of @fd should be a separate patch, both to
reduce churn and because it's not a risk free change, e.g. if this is the improper
size then bisection should point at the fdname patch, not at this combined patch.

	char fdname[ITOA_MAX_LEN + 1];

> 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>
> ---

...

> @@ -4774,6 +4781,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;
> @@ -4782,7 +4790,8 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
>  	if (fd < 0)
>  		return fd;
>  
> -	kvm = kvm_create_vm(type);
> +	snprintf(fdname, sizeof(fdname), "%d", fd);

Nit, I'd prefer a blank line here so that it's easier to see the call to
kvm_create_vm().

> +	kvm = kvm_create_vm(type, fdname);
>  	if (IS_ERR(kvm)) {
>  		r = PTR_ERR(kvm);
>  		goto put_fd;
> @@ -4799,17 +4808,6 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
>  		goto put_kvm;
>  	}
>  
> -	/*
> -	 * Don't call kvm_put_kvm anymore at this point; file->f_op is
> -	 * already set, with ->release() being kvm_vm_release().  In error
> -	 * cases it will be called by the final fput(file) and will take
> -	 * care of doing kvm_put_kvm(kvm).
> -	 */

I think we should keep the comment to warn future developers.  I'm tempted to say
it could be reworded to say something like "if you're adding a call that can fail
at this point, you're doing it wrong".  But for this patch, I'd say just leave the
comment intact.

> -	if (kvm_create_vm_debugfs(kvm, r) < 0) {
> -		fput(file);
> -		r = -ENOMEM;
> -		goto put_fd;
> -	}
>  	kvm_uevent_notify_change(KVM_EVENT_CREATE_VM, kvm);
>  
>  	fd_install(fd, file);
> -- 
> 2.36.1.124.g0e6072fb45-goog
> 
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 5/5] KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> 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>

Heh, with the above changelog, shouldn't this be?  :-)

  Signed-off-by: Oliver "Works on my Machine" Upton <oupton@google.com>

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

Re: [PATCH v2 5/5] KVM: Hoist debugfs_dentry init to kvm_create_vm_debugfs() (again)

[Sean Christopherson]

On Wed, May 18, 2022, Oliver Upton wrote:
> 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>

Heh, with the above changelog, shouldn't this be?  :-)

  Signed-off-by: Oliver "Works on my Machine" Upton <oupton@google.com>

Reviewed-by: Sean Christopherson <seanjc@google.com>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm