Re: [PATCH 3/8] powerpc/nvram: Move an assignment for the variable "ret" in dev_nvram_write()

[Tyrel Datwyler <tyreld@linux.vnet.ibm.com>]

On 01/19/2017 08:56 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Thu, 19 Jan 2017 15:55:36 +0100
> 
> A local variable was set to an error code before a concrete error situation
> was detected. Thus move the corresponding assignment into an if branch
> to indicate a software failure there.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  arch/powerpc/kernel/nvram_64.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
> index cf839adf3aa7..dc90a0e9ad65 100644
> --- a/arch/powerpc/kernel/nvram_64.c
> +++ b/arch/powerpc/kernel/nvram_64.c
> @@ -806,9 +806,10 @@ static ssize_t dev_nvram_write(struct file *file, const char __user *buf,
>  	if (!tmp)
>  		return -ENOMEM;
> 
> -	ret = -EFAULT;
> -	if (copy_from_user(tmp, buf, count))
> +	if (copy_from_user(tmp, buf, count)) {
> +		ret = -EFAULT;
>  		goto out;
> +	}
>
>  	ret = ppc_md.nvram_write(tmp, count, ppos);
> 

I think you really could have squashed patches 1-3 into a single patch
that returns directly after any failure. After this 3rd patch this is
now the only spot that branches to the "out" label. At this point you
might as well remove that label and move the kfree(tmp) call up and
return directly after the failure and at the nvram_write() call site
doing away completely with the "ret" variable.

Something like the following patch:

diff --git a/arch/powerpc/kernel/nvram_64.c b/arch/powerpc/kernel/nvram_64.c
index d5e2b83..eadb55c 100644
--- a/arch/powerpc/kernel/nvram_64.c
+++ b/arch/powerpc/kernel/nvram_64.c
@@ -789,37 +789,29 @@ static ssize_t dev_nvram_read(struct file *file,
char __user *buf,
 static ssize_t dev_nvram_write(struct file *file, const char __user *buf,
                          size_t count, loff_t *ppos)
 {
-       ssize_t ret;
-       char *tmp = NULL;
+       char *tmp;
        ssize_t size;

-       ret = -ENODEV;
        if (!ppc_md.nvram_size)
-               goto out;
+               return -ENODEV;

-       ret = 0;
        size = ppc_md.nvram_size();
        if (*ppos >= size || size < 0)
-               goto out;
+               return 0;

        count = min_t(size_t, count, size - *ppos);
        count = min(count, PAGE_SIZE);

-       ret = -ENOMEM;
        tmp = kmalloc(count, GFP_KERNEL);
        if (!tmp)
-               goto out;
-
-       ret = -EFAULT;
-       if (copy_from_user(tmp, buf, count))
-               goto out;
-
-       ret = ppc_md.nvram_write(tmp, count, ppos);
+               return -ENOMEM;

-out:
-       kfree(tmp);
-       return ret;
+       if (copy_from_user(tmp, buf, count)) {
+               kfree(tmp);
+               return -EFAULT;
+       }

+       return ppc_md.nvram_write(tmp, count, ppos);
 }

 static long dev_nvram_ioctl(struct file *file, unsigned int cmd,