| Message ID | 20190307231839.3330-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 3e14592da654d53d87987aa09753d5a26e45446f |
| Headers | show |
| Series | scsi: gdth: Only call dma_free_coherent when buf is not NULL in ioc_general | expand |
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)))
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
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
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
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
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.
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; }
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(-)