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

[Sean Christopherson <seanjc@google.com>]

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