[PATCH] IB/rdmavt: Remove err declaration in if statement in rvt_create_cq

[PATCH] IB/rdmavt: Remove err declaration in if statement in rvt_create_cq

[Nathan Chancellor]

clang warns:

drivers/infiniband/sw/rdmavt/cq.c:260:7: warning: variable 'err' is used
uninitialized whenever 'if' condition is true
[-Wsometimes-uninitialized]
                if (err)
                    ^~~
drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs
here
        return err;
               ^~~
drivers/infiniband/sw/rdmavt/cq.c:260:3: note: remove the 'if' if its
condition is always false
                if (err)
                ^~~~~~~~
drivers/infiniband/sw/rdmavt/cq.c:253:7: warning: variable 'err' is used
uninitialized whenever 'if' condition is true
[-Wsometimes-uninitialized]
                if (!cq->ip) {
                    ^~~~~~~
drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs
here
        return err;
               ^~~
drivers/infiniband/sw/rdmavt/cq.c:253:3: note: remove the 'if' if its
condition is always false
                if (!cq->ip) {
                ^~~~~~~~~~~~~~
drivers/infiniband/sw/rdmavt/cq.c:211:9: note: initialize the variable
'err' to silence this warning
        int err;
               ^
                = 0
2 warnings generated.

There are two err declarations in this function: at the top and within
an if statement; clang warns because the assignments to err in the if
statement are local to the if statement so err will be used
uninitialized if this error handling is used. Remove the if statement's
err declaration so that everything works properly.

Fixes: 239b0e52d8aa ("IB/hfi1: Move rvt_cq_wc struct into uapi directory")
Link: https://github.com/ClangBuiltLinux/linux/issues/594
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/infiniband/sw/rdmavt/cq.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/infiniband/sw/rdmavt/cq.c b/drivers/infiniband/sw/rdmavt/cq.c
index fac87b13329d..a85571a4cf57 100644
--- a/drivers/infiniband/sw/rdmavt/cq.c
+++ b/drivers/infiniband/sw/rdmavt/cq.c
@@ -247,8 +247,6 @@ int rvt_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 	 * See rvt_mmap() for details.
 	 */
 	if (udata && udata->outlen >= sizeof(__u64)) {
-		int err;
-
 		cq->ip = rvt_create_mmap_info(rdi, sz, udata, u_wc);
 		if (!cq->ip) {
 			err = -ENOMEM;
-- 
2.22.0

Re: [PATCH] IB/rdmavt: Remove err declaration in if statement in rvt_create_cq

[Nick Desaulniers]

On Tue, Jul 9, 2019 at 3:13 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> clang warns:
>
> drivers/infiniband/sw/rdmavt/cq.c:260:7: warning: variable 'err' is used

Oh, !$*@, this is a tricky one.  While the if scoped `err` declared on
L250 is initialized when used at L260, the function scoped `err`
declared on L211 is not initialized when it is used on L310 when
control flow enters the if on L249 then the goto on L255 or L261.  So
this is a bug due to the if scoped `err` "shadowing" the function
scoped `err`.

Maybe not important enough to send a v2, but I feel like the commit
message should say something along the lines of `fix a potential use
of uninitialized memory due to shadowing`.  Either way, this fixes a
real bug, so thanks for the patch.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
>                 if (err)
>                     ^~~
> drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs
> here
>         return err;
>                ^~~
> drivers/infiniband/sw/rdmavt/cq.c:260:3: note: remove the 'if' if its
> condition is always false
>                 if (err)
>                 ^~~~~~~~
> drivers/infiniband/sw/rdmavt/cq.c:253:7: warning: variable 'err' is used
> uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
>                 if (!cq->ip) {
>                     ^~~~~~~
> drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs
> here
>         return err;
>                ^~~
> drivers/infiniband/sw/rdmavt/cq.c:253:3: note: remove the 'if' if its
> condition is always false
>                 if (!cq->ip) {
>                 ^~~~~~~~~~~~~~
> drivers/infiniband/sw/rdmavt/cq.c:211:9: note: initialize the variable
> 'err' to silence this warning
>         int err;
>                ^
>                 = 0
> 2 warnings generated.
>
> There are two err declarations in this function: at the top and within
> an if statement; clang warns because the assignments to err in the if
> statement are local to the if statement so err will be used
> uninitialized if this error handling is used. Remove the if statement's
> err declaration so that everything works properly.
>
> Fixes: 239b0e52d8aa ("IB/hfi1: Move rvt_cq_wc struct into uapi directory")
> Link: https://github.com/ClangBuiltLinux/linux/issues/594
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  drivers/infiniband/sw/rdmavt/cq.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rdmavt/cq.c b/drivers/infiniband/sw/rdmavt/cq.c
> index fac87b13329d..a85571a4cf57 100644
> --- a/drivers/infiniband/sw/rdmavt/cq.c
> +++ b/drivers/infiniband/sw/rdmavt/cq.c
> @@ -247,8 +247,6 @@ int rvt_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
>          * See rvt_mmap() for details.
>          */
>         if (udata && udata->outlen >= sizeof(__u64)) {
> -               int err;
> -

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] IB/rdmavt: Remove err declaration in if statement in rvt_create_cq

[Nathan Chancellor]

On Tue, Jul 09, 2019 at 03:38:57PM -0700, Nick Desaulniers wrote:
> On Tue, Jul 9, 2019 at 3:13 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > clang warns:
> >
> > drivers/infiniband/sw/rdmavt/cq.c:260:7: warning: variable 'err' is used
> 
> Oh, !$*@, this is a tricky one.  While the if scoped `err` declared on
> L250 is initialized when used at L260, the function scoped `err`
> declared on L211 is not initialized when it is used on L310 when
> control flow enters the if on L249 then the goto on L255 or L261.  So
> this is a bug due to the if scoped `err` "shadowing" the function
> scoped `err`.
> 
> Maybe not important enough to send a v2, but I feel like the commit
> message should say something along the lines of `fix a potential use
> of uninitialized memory due to shadowing`.  Either way, this fixes a
> real bug, so thanks for the patch.
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Gah, shadowing was the word I was looking for. Knew the behavior, didn't
remember what it was called. I like the clarity of your explanation
better so I will clean up the message and send a v2 shortly with your
reviewed by.

Thanks for the review!
Nathan

[PATCH v2] IB/rdmavt: Fix variable shadowing issue in rvt_create_cq

[Nathan Chancellor]

clang warns:

drivers/infiniband/sw/rdmavt/cq.c:260:7: warning: variable 'err' is used
uninitialized whenever 'if' condition is true
[-Wsometimes-uninitialized]
                if (err)
                    ^~~
drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs
here
        return err;
               ^~~
drivers/infiniband/sw/rdmavt/cq.c:260:3: note: remove the 'if' if its
condition is always false
                if (err)
                ^~~~~~~~
drivers/infiniband/sw/rdmavt/cq.c:253:7: warning: variable 'err' is used
uninitialized whenever 'if' condition is true
[-Wsometimes-uninitialized]
                if (!cq->ip) {
                    ^~~~~~~
drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs
here
        return err;
               ^~~
drivers/infiniband/sw/rdmavt/cq.c:253:3: note: remove the 'if' if its
condition is always false
                if (!cq->ip) {
                ^~~~~~~~~~~~~~
drivers/infiniband/sw/rdmavt/cq.c:211:9: note: initialize the variable
'err' to silence this warning
        int err;
               ^
                = 0
2 warnings generated.

The function scoped err variable is uninitialized when the flow jumps
into the if statement. The if scoped err variable shadows the function
scoped err variable, preventing the err assignments within the if
statement to be reflected at the function level, which will cause
uninitialized use when the goto statements are taken.

Just remove the if scoped err declaration so that there is only one
copy of the err variable for this function.

Fixes: 239b0e52d8aa ("IB/hfi1: Move rvt_cq_wc struct into uapi directory")
Link: https://github.com/ClangBuiltLinux/linux/issues/594
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Updated the wording of the commit message to use proper terms like
  scoping and shadowing, thanks to review from Nick (let me know if the
  wording isn't up to snuff).

 drivers/infiniband/sw/rdmavt/cq.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/infiniband/sw/rdmavt/cq.c b/drivers/infiniband/sw/rdmavt/cq.c
index fac87b13329d..a85571a4cf57 100644
--- a/drivers/infiniband/sw/rdmavt/cq.c
+++ b/drivers/infiniband/sw/rdmavt/cq.c
@@ -247,8 +247,6 @@ int rvt_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 	 * See rvt_mmap() for details.
 	 */
 	if (udata && udata->outlen >= sizeof(__u64)) {
-		int err;
-
 		cq->ip = rvt_create_mmap_info(rdi, sz, udata, u_wc);
 		if (!cq->ip) {
 			err = -ENOMEM;
-- 
2.22.0

Re: [PATCH v2] IB/rdmavt: Fix variable shadowing issue in rvt_create_cq

[Nick Desaulniers]

On Tue, Jul 9, 2019 at 4:06 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
> The function scoped err variable is uninitialized when the flow jumps
> into the if statement. The if scoped err variable shadows the function
> scoped err variable, preventing the err assignments within the if
> statement to be reflected at the function level, which will cause
> uninitialized use when the goto statements are taken.
>
> Just remove the if scoped err declaration so that there is only one
> copy of the err variable for this function.
>
> Fixes: 239b0e52d8aa ("IB/hfi1: Move rvt_cq_wc struct into uapi directory")
> Link: https://github.com/ClangBuiltLinux/linux/issues/594
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2:
>
> * Updated the wording of the commit message to use proper terms like
>   scoping and shadowing, thanks to review from Nick (let me know if the
>   wording isn't up to snuff).

LGTM thanks for following up w/ v2.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] IB/rdmavt: Fix variable shadowing issue in rvt_create_cq

[Jason Gunthorpe]

On Tue, Jul 09, 2019 at 04:05:53PM -0700, Nathan Chancellor wrote:
> clang warns:
> 
> drivers/infiniband/sw/rdmavt/cq.c:260:7: warning: variable 'err' is used
> uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
>                 if (err)
>                     ^~~
> drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs
> here
>         return err;
>                ^~~
> drivers/infiniband/sw/rdmavt/cq.c:260:3: note: remove the 'if' if its
> condition is always false
>                 if (err)
>                 ^~~~~~~~
> drivers/infiniband/sw/rdmavt/cq.c:253:7: warning: variable 'err' is used
> uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
>                 if (!cq->ip) {
>                     ^~~~~~~
> drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs
> here
>         return err;
>                ^~~
> drivers/infiniband/sw/rdmavt/cq.c:253:3: note: remove the 'if' if its
> condition is always false
>                 if (!cq->ip) {
>                 ^~~~~~~~~~~~~~
> drivers/infiniband/sw/rdmavt/cq.c:211:9: note: initialize the variable
> 'err' to silence this warning
>         int err;
>                ^
>                 = 0
> 2 warnings generated.
> 
> The function scoped err variable is uninitialized when the flow jumps
> into the if statement. The if scoped err variable shadows the function
> scoped err variable, preventing the err assignments within the if
> statement to be reflected at the function level, which will cause
> uninitialized use when the goto statements are taken.
> 
> Just remove the if scoped err declaration so that there is only one
> copy of the err variable for this function.
> 
> Fixes: 239b0e52d8aa ("IB/hfi1: Move rvt_cq_wc struct into uapi directory")
> Link: https://github.com/ClangBuiltLinux/linux/issues/594
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>

Applied to for-next with Mike's ack

Thanks,
Jason

Re: [PATCH v2] IB/rdmavt: Fix variable shadowing issue in rvt_create_cq

[Ira Weiny]

On Tue, Jul 09, 2019 at 04:05:53PM -0700, Nathan Chancellor wrote:
> clang warns:
> 
> drivers/infiniband/sw/rdmavt/cq.c:260:7: warning: variable 'err' is used
> uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
>                 if (err)
>                     ^~~
> drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs
> here
>         return err;
>                ^~~
> drivers/infiniband/sw/rdmavt/cq.c:260:3: note: remove the 'if' if its
> condition is always false
>                 if (err)
>                 ^~~~~~~~
> drivers/infiniband/sw/rdmavt/cq.c:253:7: warning: variable 'err' is used
> uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
>                 if (!cq->ip) {
>                     ^~~~~~~
> drivers/infiniband/sw/rdmavt/cq.c:310:9: note: uninitialized use occurs
> here
>         return err;
>                ^~~
> drivers/infiniband/sw/rdmavt/cq.c:253:3: note: remove the 'if' if its
> condition is always false
>                 if (!cq->ip) {
>                 ^~~~~~~~~~~~~~
> drivers/infiniband/sw/rdmavt/cq.c:211:9: note: initialize the variable
> 'err' to silence this warning
>         int err;
>                ^
>                 = 0
> 2 warnings generated.

What version of the kernel was this found on?

I don't see the problem with 5.2.  AFAICS there is no 'err' in the function
scope and the if scoped 'err' is initialized properly on line 239.

Ira

> 
> The function scoped err variable is uninitialized when the flow jumps
> into the if statement. The if scoped err variable shadows the function
> scoped err variable, preventing the err assignments within the if
> statement to be reflected at the function level, which will cause
> uninitialized use when the goto statements are taken.
> 
> Just remove the if scoped err declaration so that there is only one
> copy of the err variable for this function.
> 
> Fixes: 239b0e52d8aa ("IB/hfi1: Move rvt_cq_wc struct into uapi directory")
> Link: https://github.com/ClangBuiltLinux/linux/issues/594
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> 
> v1 -> v2:
> 
> * Updated the wording of the commit message to use proper terms like
>   scoping and shadowing, thanks to review from Nick (let me know if the
>   wording isn't up to snuff).
> 
>  drivers/infiniband/sw/rdmavt/cq.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/infiniband/sw/rdmavt/cq.c b/drivers/infiniband/sw/rdmavt/cq.c
> index fac87b13329d..a85571a4cf57 100644
> --- a/drivers/infiniband/sw/rdmavt/cq.c
> +++ b/drivers/infiniband/sw/rdmavt/cq.c
> @@ -247,8 +247,6 @@ int rvt_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
>  	 * See rvt_mmap() for details.
>  	 */
>  	if (udata && udata->outlen >= sizeof(__u64)) {
> -		int err;
> -
>  		cq->ip = rvt_create_mmap_info(rdi, sz, udata, u_wc);
>  		if (!cq->ip) {
>  			err = -ENOMEM;
> -- 
> 2.22.0
> 

Re: [PATCH v2] IB/rdmavt: Fix variable shadowing issue in rvt_create_cq

[Nathan Chancellor]

On Wed, Jul 10, 2019 at 10:03:23AM -0700, Ira Weiny wrote:
> What version of the kernel was this found on?
> 
> I don't see the problem with 5.2.  AFAICS there is no 'err' in the function
> scope and the if scoped 'err' is initialized properly on line 239.
> 
> Ira
> 

$ git describe --contains 239b0e52d8aa
next-20190709~84^2~57

I should probably be better about adding 'PATCH -next' to my patches,
sorry for the confusion!

Cheers,
Nathan

Re: [PATCH v2] IB/rdmavt: Fix variable shadowing issue in rvt_create_cq

[Nathan Chancellor]

On Wed, Jul 10, 2019 at 02:02:16PM -0300, Jason Gunthorpe wrote:
> 
> Applied to for-next with Mike's ack
> 
> Thanks,
> Jason

Thank you Jason, much appreciated!

Nathan

Re: [PATCH v2] IB/rdmavt: Fix variable shadowing issue in rvt_create_cq

[Ira Weiny]

On Wed, Jul 10, 2019 at 10:07:11AM -0700, Nathan Chancellor wrote:
> On Wed, Jul 10, 2019 at 10:03:23AM -0700, Ira Weiny wrote:
> > What version of the kernel was this found on?
> > 
> > I don't see the problem with 5.2.  AFAICS there is no 'err' in the function
> > scope and the if scoped 'err' is initialized properly on line 239.
> > 
> > Ira
> > 
> 
> $ git describe --contains 239b0e52d8aa
> next-20190709~84^2~57
> 
> I should probably be better about adding 'PATCH -next' to my patches,
> sorry for the confusion!

Ah I see it now...  Sorry I was just not up to date on the rdma tree.  Sorry
about the noise.

Thanks,
Ira

> 
> Cheers,
> Nathan