Re: [PATCH v2 1/3] KVM: Don't create VM debugfs files outside of the VM directory

[Marc Zyngier <maz@kernel.org>]

Hi Oliver,

On Mon, 04 Apr 2022 19:21:17 +0100,
Oliver Upton <oupton@google.com> wrote:
> 
> Unfortunately, there is no guarantee that KVM was able to instantiate a
> debugfs directory for a particular VM. To that end, KVM shouldn't even
> attempt to create new debugfs files in this case. If the specified
> parent dentry is NULL, debugfs_create_file() will instantiate files at
> the root of debugfs.
> 
> For arm64, it is possible to create the vgic-state file outside of a
> VM directory, the file is not cleaned up when a VM is destroyed.
> Nonetheless, the corresponding struct kvm is freed when the VM is
> destroyed.
> 
> Nip the problem in the bud for all possible errant debugfs file
> creations by initializing kvm->debugfs_dentry to -ENOENT. In so doing,
> debugfs_create_file() will fail instead of creating the file in the root
> directory.
> 
> Cc: stable@kernel.org
> Fixes: 929f45e32499 ("kvm: no need to check return value of debugfs_create functions")
> Signed-off-by: Oliver Upton <oupton@google.com>
> ---
>  virt/kvm/kvm_main.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 70e05af5ebea..04a426e65cb8 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -932,7 +932,7 @@ static void kvm_destroy_vm_debugfs(struct kvm *kvm)
>  	int kvm_debugfs_num_entries = kvm_vm_stats_header.num_desc +
>  				      kvm_vcpu_stats_header.num_desc;
>  
> -	if (!kvm->debugfs_dentry)
> +	if (!IS_ERR(kvm->debugfs_dentry))
>  		return;

Shouldn't this condition be inverted? It certainly looks odd.

>  
>  	debugfs_remove_recursive(kvm->debugfs_dentry);
> @@ -955,6 +955,12 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd)
>  	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;
>  
> @@ -5479,7 +5485,7 @@ static void kvm_uevent_notify_change(unsigned int type, struct kvm *kvm)
>  	}
>  	add_uevent_var(env, "PID=%d", kvm->userspace_pid);
>  
> -	if (kvm->debugfs_dentry) {
> +	if (!IS_ERR(kvm->debugfs_dentry)) {
>  		char *tmp, *p = kmalloc(PATH_MAX, GFP_KERNEL_ACCOUNT);
>  
>  		if (p) {

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.