[PATCH] scsi: gdth: Only call dma_free_coherent when buf is not NULL in ioc_general

[PATCH] scsi: gdth: Only call dma_free_coherent when buf is not NULL in ioc_general

[Nathan Chancellor]

When building with -Wsometimes-uninitialized, Clang warns:

drivers/scsi/gdth.c:3662:6: warning: variable 'paddr' is used
uninitialized whenever 'if' condition is false
[-Wsometimes-uninitialized]

Don't attempt to call dma_free_coherent when buf is NULL (meaning that
we never called dma_alloc_coherent and initialized paddr), which avoids
this warning.

Link: https://github.com/ClangBuiltLinux/linux/issues/402
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/scsi/gdth.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index e7f1dd4f3b66..0ca9b4393770 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -3697,8 +3697,9 @@ static int ioc_general(void __user *arg, char *cmnd)
 
 	rval = 0;
 out_free_buf:
-	dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
-			paddr);
+	if (buf)
+		dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len,
+				  buf, paddr);
 	return rval;
 }
  
-- 
2.21.0

Re: [PATCH] scsi: gdth: Only call dma_free_coherent when buf is not NULL in ioc_general

[Nick Desaulniers]

On Thu, Mar 7, 2019 at 3:19 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/scsi/gdth.c:3662:6: warning: variable 'paddr' is used
> uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
>
> Don't attempt to call dma_free_coherent when buf is NULL (meaning that
> we never called dma_alloc_coherent and initialized paddr), which avoids
> this warning.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/402
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/scsi/gdth.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index e7f1dd4f3b66..0ca9b4393770 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -3697,8 +3697,9 @@ static int ioc_general(void __user *arg, char *cmnd)
>
>         rval = 0;
>  out_free_buf:
> -       dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
> -                       paddr);
> +       if (buf)
> +               dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len,
> +                                 buf, paddr);
>         return rval;
>  }
>
> --
> 2.21.0
>

Alternatively, paddr is a dma_addr_t defined in include/linux/types.h:

#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
typedef u64 dma_addr_t;
#else
typedef u32 dma_addr_t;
#endif

Just initializing it to zero might be simpler than complicating the
control flow of this function further. Thoughts?

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index e7f1dd4f3b66..5a3f849ebf64 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -3643,7 +3643,7 @@ static int ioc_general(void __user *arg, char *cmnd)
        gdth_ioctl_general gen;
        gdth_ha_str *ha;
        char *buf = NULL;
-       dma_addr_t paddr;
+       dma_addr_t paddr = 0U;
        int rval;

        if (copy_from_user(&gen, arg, sizeof(gdth_ioctl_general)))
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] scsi: gdth: Only call dma_free_coherent when buf is not NULL in ioc_general

[Nathan Chancellor]

On Fri, Mar 08, 2019 at 01:14:25PM -0800, Nick Desaulniers wrote:
> On Thu, Mar 7, 2019 at 3:19 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > When building with -Wsometimes-uninitialized, Clang warns:
> >
> > drivers/scsi/gdth.c:3662:6: warning: variable 'paddr' is used
> > uninitialized whenever 'if' condition is false
> > [-Wsometimes-uninitialized]
> >
> > Don't attempt to call dma_free_coherent when buf is NULL (meaning that
> > we never called dma_alloc_coherent and initialized paddr), which avoids
> > this warning.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/402
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/scsi/gdth.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> > index e7f1dd4f3b66..0ca9b4393770 100644
> > --- a/drivers/scsi/gdth.c
> > +++ b/drivers/scsi/gdth.c
> > @@ -3697,8 +3697,9 @@ static int ioc_general(void __user *arg, char *cmnd)
> >
> >         rval = 0;
> >  out_free_buf:
> > -       dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
> > -                       paddr);
> > +       if (buf)
> > +               dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len,
> > +                                 buf, paddr);
> >         return rval;
> >  }
> >
> > --
> > 2.21.0
> >
> 
> Alternatively, paddr is a dma_addr_t defined in include/linux/types.h:
> 
> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> typedef u64 dma_addr_t;
> #else
> typedef u32 dma_addr_t;
> #endif
> 
> Just initializing it to zero might be simpler than complicating the
> control flow of this function further. Thoughts?
> 
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index e7f1dd4f3b66..5a3f849ebf64 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -3643,7 +3643,7 @@ static int ioc_general(void __user *arg, char *cmnd)
>         gdth_ioctl_general gen;
>         gdth_ha_str *ha;
>         char *buf = NULL;
> -       dma_addr_t paddr;
> +       dma_addr_t paddr = 0U;
>         int rval;
> 
>         if (copy_from_user(&gen, arg, sizeof(gdth_ioctl_general)))
> -- 
> Thanks,
> ~Nick Desaulniers

I suppose it depends on if it's okay to call dma_free_coherent without
dma_alloc_coherent. I did a scan of the tree before sending this out
and pretty much all sites that have error handling check that the third
parameter is not NULL before calling it.

I should have added Christoph to this thread when I initially sent it
out since commit 9f475ebff8e4 ("scsi: gdth: refactor ioc_general")
introduced this. Done now.

Cheers,
Nathan

Re: [PATCH] scsi: gdth: Only call dma_free_coherent when buf is not NULL in ioc_general

[Nathan Chancellor]

On Thu, Mar 07, 2019 at 04:18:39PM -0700, Nathan Chancellor wrote:
> When building with -Wsometimes-uninitialized, Clang warns:
> 
> drivers/scsi/gdth.c:3662:6: warning: variable 'paddr' is used
> uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
> 
> Don't attempt to call dma_free_coherent when buf is NULL (meaning that
> we never called dma_alloc_coherent and initialized paddr), which avoids
> this warning.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/402
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/scsi/gdth.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index e7f1dd4f3b66..0ca9b4393770 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -3697,8 +3697,9 @@ static int ioc_general(void __user *arg, char *cmnd)
>  
>  	rval = 0;
>  out_free_buf:
> -	dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
> -			paddr);
> +	if (buf)
> +		dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len,
> +				  buf, paddr);
>  	return rval;
>  }
>   
> -- 
> 2.21.0
> 

Gentle ping (if there was a response to this, I didn't receive it). I
know I sent it in the middle of a merge window so I get if it slipped
through the cracks.

Thanks,
Nathan

Re: [PATCH] scsi: gdth: Only call dma_free_coherent when buf is not NULL in ioc_general

[Arnd Bergmann]

On Fri, Mar 8, 2019 at 10:14 PM 'Nick Desaulniers' via Clang Built
Linux <clang-built-linux@googlegroups.com> wrote:
>
> On Thu, Mar 7, 2019 at 3:19 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > When building with -Wsometimes-uninitialized, Clang warns:
> >
> > drivers/scsi/gdth.c:3662:6: warning: variable 'paddr' is used
> > uninitialized whenever 'if' condition is false
> > [-Wsometimes-uninitialized]
> >
> > Don't attempt to call dma_free_coherent when buf is NULL (meaning that
> > we never called dma_alloc_coherent and initialized paddr), which avoids
> > this warning.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/402
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> >  drivers/scsi/gdth.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> > index e7f1dd4f3b66..0ca9b4393770 100644
> > --- a/drivers/scsi/gdth.c
> > +++ b/drivers/scsi/gdth.c
> > @@ -3697,8 +3697,9 @@ static int ioc_general(void __user *arg, char *cmnd)
> >
> >         rval = 0;
> >  out_free_buf:
> > -       dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
> > -                       paddr);
> > +       if (buf)
> > +               dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len,
> > +                                 buf, paddr);
> >         return rval;
> >  }

I came up with a different fix for this, but I think yours is better

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

For reference, this was my version:

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index e7f1dd4f3b66..c01f243902e1 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -3697,8 +3697,9 @@ static int ioc_general(void __user *arg, char *cmnd)

        rval = 0;
 out_free_buf:
-       dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
-                       paddr);
+       if (gen.data_len + gen.sense_len > 0)
+               dma_free_coherent(&ha->pdev->dev, gen.data_len +
gen.sense_len, buf,
+                               paddr);
        return rval;
 }


> Alternatively, paddr is a dma_addr_t defined in include/linux/types.h:
>
> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> typedef u64 dma_addr_t;
> #else
> typedef u32 dma_addr_t;
> #endif
>
> Just initializing it to zero might be simpler than complicating the
> control flow of this function further. Thoughts?

No, blindly shutting up warnings is almost never the
right solution, even when they are false positives. The code
might change in the future and the bogus initialization would
then prevent the compiler from warning about a new bug.

      Arnd

Re: [PATCH] scsi: gdth: Only call dma_free_coherent when buf is not NULL in ioc_general

[Nathan Chancellor]

On Fri, Mar 22, 2019 at 03:30:08PM +0100, Arnd Bergmann wrote:
> On Fri, Mar 8, 2019 at 10:14 PM 'Nick Desaulniers' via Clang Built
> Linux <clang-built-linux@googlegroups.com> wrote:
> >
> > On Thu, Mar 7, 2019 at 3:19 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > When building with -Wsometimes-uninitialized, Clang warns:
> > >
> > > drivers/scsi/gdth.c:3662:6: warning: variable 'paddr' is used
> > > uninitialized whenever 'if' condition is false
> > > [-Wsometimes-uninitialized]
> > >
> > > Don't attempt to call dma_free_coherent when buf is NULL (meaning that
> > > we never called dma_alloc_coherent and initialized paddr), which avoids
> > > this warning.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/402
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > >  drivers/scsi/gdth.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> > > index e7f1dd4f3b66..0ca9b4393770 100644
> > > --- a/drivers/scsi/gdth.c
> > > +++ b/drivers/scsi/gdth.c
> > > @@ -3697,8 +3697,9 @@ static int ioc_general(void __user *arg, char *cmnd)
> > >
> > >         rval = 0;
> > >  out_free_buf:
> > > -       dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
> > > -                       paddr);
> > > +       if (buf)
> > > +               dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len,
> > > +                                 buf, paddr);
> > >         return rval;
> > >  }
> 
> I came up with a different fix for this, but I think yours is better
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
> For reference, this was my version:

This is also what I had initially but I felt this was more future proof
and matches how the rest of the tree handles calls to dma_free_coherent.

Thanks for the review!
Nathan

> 
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index e7f1dd4f3b66..c01f243902e1 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -3697,8 +3697,9 @@ static int ioc_general(void __user *arg, char *cmnd)
> 
>         rval = 0;
>  out_free_buf:
> -       dma_free_coherent(&ha->pdev->dev, gen.data_len + gen.sense_len, buf,
> -                       paddr);
> +       if (gen.data_len + gen.sense_len > 0)
> +               dma_free_coherent(&ha->pdev->dev, gen.data_len +
> gen.sense_len, buf,
> +                               paddr);
>         return rval;
>  }
> 
> 
> > Alternatively, paddr is a dma_addr_t defined in include/linux/types.h:
> >
> > #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> > typedef u64 dma_addr_t;
> > #else
> > typedef u32 dma_addr_t;
> > #endif
> >
> > Just initializing it to zero might be simpler than complicating the
> > control flow of this function further. Thoughts?
> 
> No, blindly shutting up warnings is almost never the
> right solution, even when they are false positives. The code
> might change in the future and the bogus initialization would
> then prevent the compiler from warning about a new bug.
> 
>       Arnd

Re: [PATCH] scsi: gdth: Only call dma_free_coherent when buf is not NULL in ioc_general

[Martin K. Petersen]

Nathan,

> When building with -Wsometimes-uninitialized, Clang warns:
>
> drivers/scsi/gdth.c:3662:6: warning: variable 'paddr' is used
> uninitialized whenever 'if' condition is false
> [-Wsometimes-uninitialized]
>
> Don't attempt to call dma_free_coherent when buf is NULL (meaning that
> we never called dma_alloc_coherent and initialized paddr), which avoids
> this warning.

Applied to 5.2/scsi-queue, thanks.

-- 
Martin K. Petersen	Oracle Linux Engineering