linux-kernel - Re: [PATCH 2/6] powerpc: kvm: no need to check return value of debugfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 03 Mar 2020 18:46:23 +1100
From:   Michael Ellerman <mpe@...erman.id.au>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linuxppc-dev@...ts.ozlabs.org
Cc:     linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Paul Mackerras <paulus@...abs.org>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        kvm-ppc@...r.kernel.org
Subject: Re: [PATCH 2/6] powerpc: kvm: no need to check return value of debugfs_create functions

Greg Kroah-Hartman <gregkh@...uxfoundation.org> writes:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.

Except it does need to do something different, if the file was created
it needs to be removed in the remove path.

> diff --git a/arch/powerpc/kvm/timing.c b/arch/powerpc/kvm/timing.c
> index bfe4f106cffc..8e4791c6f2af 100644
> --- a/arch/powerpc/kvm/timing.c
> +++ b/arch/powerpc/kvm/timing.c
> @@ -207,19 +207,12 @@ static const struct file_operations kvmppc_exit_timing_fops = {
>  void kvmppc_create_vcpu_debugfs(struct kvm_vcpu *vcpu, unsigned int id)
>  {
>  	static char dbg_fname[50];
> -	struct dentry *debugfs_file;
>  
>  	snprintf(dbg_fname, sizeof(dbg_fname), "vm%u_vcpu%u_timing",
>  		 current->pid, id);
> -	debugfs_file = debugfs_create_file(dbg_fname, 0666,
> -					kvm_debugfs_dir, vcpu,
> -					&kvmppc_exit_timing_fops);
> -
> -	if (!debugfs_file) {
> -		printk(KERN_ERR"%s: error creating debugfs file %s\n",
> -			__func__, dbg_fname);
> -		return;
> -	}
> +	debugfs_create_file(dbg_fname, 0666, kvm_debugfs_dir, vcpu,
> +			    &kvmppc_exit_timing_fops);
> +
>  
>  	vcpu->arch.debugfs_exit_timing = debugfs_file;
>  }

This doesn't build:

    arch/powerpc/kvm/timing.c:217:35: error: 'debugfs_file' undeclared (first use in this function); did you mean 'debugfs_file_put'?

We can't just drop the assignment, we need the dentry to do the removal:

void kvmppc_remove_vcpu_debugfs(struct kvm_vcpu *vcpu)
{
	if (vcpu->arch.debugfs_exit_timing) {
		debugfs_remove(vcpu->arch.debugfs_exit_timing);
		vcpu->arch.debugfs_exit_timing = NULL;
	}
}


I squashed this in, which seems to work:

diff --git a/arch/powerpc/kvm/timing.c b/arch/powerpc/kvm/timing.c
index 8e4791c6f2af..5b7a66f86bd5 100644
--- a/arch/powerpc/kvm/timing.c
+++ b/arch/powerpc/kvm/timing.c
@@ -207,19 +207,19 @@ static const struct file_operations kvmppc_exit_timing_fops = {
 void kvmppc_create_vcpu_debugfs(struct kvm_vcpu *vcpu, unsigned int id)
 {
        static char dbg_fname[50];
+       struct dentry *debugfs_file;
 
        snprintf(dbg_fname, sizeof(dbg_fname), "vm%u_vcpu%u_timing",
                 current->pid, id);
-       debugfs_create_file(dbg_fname, 0666, kvm_debugfs_dir, vcpu,
-                           &kvmppc_exit_timing_fops);
-
+       debugfs_file = debugfs_create_file(dbg_fname, 0666, kvm_debugfs_dir,
+                                          vcpu, &kvmppc_exit_timing_fops);
 
        vcpu->arch.debugfs_exit_timing = debugfs_file;
 }
 
 void kvmppc_remove_vcpu_debugfs(struct kvm_vcpu *vcpu)
 {
-       if (vcpu->arch.debugfs_exit_timing) {
+       if (!IS_ERR_OR_NULL(vcpu->arch.debugfs_exit_timing)) {
                debugfs_remove(vcpu->arch.debugfs_exit_timing);
                vcpu->arch.debugfs_exit_timing = NULL;
        }


cheers