[PATCH] block: rnbd: rnbd-srv: silence uninitialized variable warning

[PATCH] block: rnbd: rnbd-srv: silence uninitialized variable warning

[Brooke Basile]

Clang warns:
	drivers/block/rnbd/rnbd-srv.c:150:6: warning: variable 'err' is used
	uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
        	if (IS_ERR(bio)) {
            	^~~~~~~~~~~
	drivers/block/rnbd/rnbd-srv.c:177:9: note: uninitialized use occurs here
		return err;
		^~~
	drivers/block/rnbd/rnbd-srv.c:126:9: note: initialize the variable 'err'
	to silence this warning
        	int err;
               	^
                	= 0

Silence this by replacing `err` with `ret`, returning ret = 0 upon
success.

Signed-off-by: Brooke Basile <brookebasile@gmail.com>
---
 drivers/block/rnbd/rnbd-srv.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
index 0fb94843a495..f515d1a048a9 100644
--- a/drivers/block/rnbd/rnbd-srv.c
+++ b/drivers/block/rnbd/rnbd-srv.c
@@ -123,10 +123,10 @@ static int process_rdma(struct rtrs_srv *sess,
 	struct rnbd_io_private *priv;
 	struct rnbd_srv_sess_dev *sess_dev;
 	u32 dev_id;
-	int err;
 	struct rnbd_dev_blk_io *io;
 	struct bio *bio;
 	short prio;
+	int ret = 0;
 
 	priv = kmalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -138,7 +138,7 @@ static int process_rdma(struct rtrs_srv *sess,
 	if (IS_ERR(sess_dev)) {
 		pr_err_ratelimited("Got I/O request on session %s for unknown device id %d\n",
 				   srv_sess->sessname, dev_id);
-		err = -ENOTCONN;
+		ret = -ENOTCONN;
 		goto err;
 	}
 
@@ -168,13 +168,13 @@ static int process_rdma(struct rtrs_srv *sess,
 
 	submit_bio(bio);
 
-	return 0;
+	return ret;
 
 sess_dev_put:
 	rnbd_put_sess_dev(sess_dev);
 err:
 	kfree(priv);
-	return err;
+	return ret;
 }
 
 static void destroy_device(struct rnbd_srv_dev *dev)
-- 
2.28.0

Re: [PATCH] block: rnbd: rnbd-srv: silence uninitialized variable warning

[Nathan Chancellor]

On Tue, Aug 18, 2020 at 12:03:18AM -0400, Brooke Basile wrote:
> Clang warns:
> 	drivers/block/rnbd/rnbd-srv.c:150:6: warning: variable 'err' is used
> 	uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
>         	if (IS_ERR(bio)) {
>             	^~~~~~~~~~~
> 	drivers/block/rnbd/rnbd-srv.c:177:9: note: uninitialized use occurs here
> 		return err;
> 		^~~
> 	drivers/block/rnbd/rnbd-srv.c:126:9: note: initialize the variable 'err'
> 	to silence this warning
>         	int err;
>                	^
>                 	= 0
> 
> Silence this by replacing `err` with `ret`, returning ret = 0 upon
> success.
> 
> Signed-off-by: Brooke Basile <brookebasile@gmail.com>
> ---
>  drivers/block/rnbd/rnbd-srv.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> index 0fb94843a495..f515d1a048a9 100644
> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -123,10 +123,10 @@ static int process_rdma(struct rtrs_srv *sess,
>  	struct rnbd_io_private *priv;
>  	struct rnbd_srv_sess_dev *sess_dev;
>  	u32 dev_id;
> -	int err;
>  	struct rnbd_dev_blk_io *io;
>  	struct bio *bio;
>  	short prio;
> +	int ret = 0;
>  
>  	priv = kmalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
> @@ -138,7 +138,7 @@ static int process_rdma(struct rtrs_srv *sess,
>  	if (IS_ERR(sess_dev)) {
>  		pr_err_ratelimited("Got I/O request on session %s for unknown device id %d\n",
>  				   srv_sess->sessname, dev_id);
> -		err = -ENOTCONN;
> +		ret = -ENOTCONN;
>  		goto err;
>  	}
>  
> @@ -168,13 +168,13 @@ static int process_rdma(struct rtrs_srv *sess,
>  
>  	submit_bio(bio);
>  
> -	return 0;
> +	return ret;
>  
>  sess_dev_put:
>  	rnbd_put_sess_dev(sess_dev);
>  err:
>  	kfree(priv);
> -	return err;
> +	return ret;
>  }
>  
>  static void destroy_device(struct rnbd_srv_dev *dev)
> -- 
> 2.28.0
> 

I don't think this is a proper fix since the root cause of the warning
appears to be that we are ignoring the return value of
rnbd_bio_map_kern. Should we not set err to that value like this
(completely untested)?

Cheers,
Nathan

diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
index 0fb94843a495..1b71cb2a885d 100644
--- a/drivers/block/rnbd/rnbd-srv.c
+++ b/drivers/block/rnbd/rnbd-srv.c
@@ -148,7 +148,8 @@ static int process_rdma(struct rtrs_srv *sess,
 	/* Generate bio with pages pointing to the rdma buffer */
 	bio = rnbd_bio_map_kern(data, sess_dev->rnbd_dev->ibd_bio_set, datalen, GFP_KERNEL);
 	if (IS_ERR(bio)) {
-		rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", PTR_ERR(bio));
+		err = PTR_ERR(bio);
+		rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", err);
 		goto sess_dev_put;
 	}
 

Re: [PATCH] block: rnbd: rnbd-srv: silence uninitialized variable warning

[Jinpu Wang]

On Tue, Aug 18, 2020 at 7:30 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, Aug 18, 2020 at 12:03:18AM -0400, Brooke Basile wrote:
> > Clang warns:
> >       drivers/block/rnbd/rnbd-srv.c:150:6: warning: variable 'err' is used
> >       uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
> >               if (IS_ERR(bio)) {
> >               ^~~~~~~~~~~
> >       drivers/block/rnbd/rnbd-srv.c:177:9: note: uninitialized use occurs here
> >               return err;
> >               ^~~
> >       drivers/block/rnbd/rnbd-srv.c:126:9: note: initialize the variable 'err'
> >       to silence this warning
> >               int err;
> >                       ^
> >                       = 0
> >
> > Silence this by replacing `err` with `ret`, returning ret = 0 upon
> > success.
> >
> > Signed-off-by: Brooke Basile <brookebasile@gmail.com>
> > ---
> >  drivers/block/rnbd/rnbd-srv.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> > index 0fb94843a495..f515d1a048a9 100644
> > --- a/drivers/block/rnbd/rnbd-srv.c
> > +++ b/drivers/block/rnbd/rnbd-srv.c
> > @@ -123,10 +123,10 @@ static int process_rdma(struct rtrs_srv *sess,
> >       struct rnbd_io_private *priv;
> >       struct rnbd_srv_sess_dev *sess_dev;
> >       u32 dev_id;
> > -     int err;
> >       struct rnbd_dev_blk_io *io;
> >       struct bio *bio;
> >       short prio;
> > +     int ret = 0;
> >
> >       priv = kmalloc(sizeof(*priv), GFP_KERNEL);
> >       if (!priv)
> > @@ -138,7 +138,7 @@ static int process_rdma(struct rtrs_srv *sess,
> >       if (IS_ERR(sess_dev)) {
> >               pr_err_ratelimited("Got I/O request on session %s for unknown device id %d\n",
> >                                  srv_sess->sessname, dev_id);
> > -             err = -ENOTCONN;
> > +             ret = -ENOTCONN;
> >               goto err;
> >       }
> >
> > @@ -168,13 +168,13 @@ static int process_rdma(struct rtrs_srv *sess,
> >
> >       submit_bio(bio);
> >
> > -     return 0;
> > +     return ret;
> >
> >  sess_dev_put:
> >       rnbd_put_sess_dev(sess_dev);
> >  err:
> >       kfree(priv);
> > -     return err;
> > +     return ret;
> >  }
> >
> >  static void destroy_device(struct rnbd_srv_dev *dev)
> > --
> > 2.28.0
> >
>
> I don't think this is a proper fix since the root cause of the warning
> appears to be that we are ignoring the return value of
> rnbd_bio_map_kern. Should we not set err to that value like this
> (completely untested)?
>
> Cheers,
> Nathan
Thanks Nathan, thanks Brooke,

I agree with Nathan, the problem is we forgot to set the err before
"goto sess_dev_put".
Nathan's patch is simpler, less code of change.
>
> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> index 0fb94843a495..1b71cb2a885d 100644
> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -148,7 +148,8 @@ static int process_rdma(struct rtrs_srv *sess,
>         /* Generate bio with pages pointing to the rdma buffer */
>         bio = rnbd_bio_map_kern(data, sess_dev->rnbd_dev->ibd_bio_set, datalen, GFP_KERNEL);
>         if (IS_ERR(bio)) {
> -               rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", PTR_ERR(bio));
> +               err = PTR_ERR(bio);
> +               rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", err);
>                 goto sess_dev_put;
>         }
>
Nathan, could you send a formal patch?

Thanks

[PATCH] block/rnbd: Ensure err is always initialized in process_rdma

[Nathan Chancellor]

Clang warns:

drivers/block/rnbd/rnbd-srv.c:150:6: warning: variable 'err' is used
uninitialized whenever 'if' condition is true
[-Wsometimes-uninitialized]
        if (IS_ERR(bio)) {
            ^~~~~~~~~~~
drivers/block/rnbd/rnbd-srv.c:177:9: note: uninitialized use occurs here
        return err;
               ^~~
drivers/block/rnbd/rnbd-srv.c:150:2: note: remove the 'if' if its
condition is always false
        if (IS_ERR(bio)) {
        ^~~~~~~~~~~~~~~~~~
drivers/block/rnbd/rnbd-srv.c:126:9: note: initialize the variable 'err'
to silence this warning
        int err;
               ^
                = 0
1 warning generated.

err is indeed uninitialized when this statement is taken. Ensure that it
is assigned the error value of bio before jumping to the error handling
label.

Fixes: 735d77d4fd28 ("rnbd: remove rnbd_dev_submit_io")
Link: https://github.com/ClangBuiltLinux/linux/issues/1134
Reported-by: Brooke Basile <brookebasile@gmail.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 drivers/block/rnbd/rnbd-srv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
index 0fb94843a495..1b71cb2a885d 100644
--- a/drivers/block/rnbd/rnbd-srv.c
+++ b/drivers/block/rnbd/rnbd-srv.c
@@ -148,7 +148,8 @@ static int process_rdma(struct rtrs_srv *sess,
 	/* Generate bio with pages pointing to the rdma buffer */
 	bio = rnbd_bio_map_kern(data, sess_dev->rnbd_dev->ibd_bio_set, datalen, GFP_KERNEL);
 	if (IS_ERR(bio)) {
-		rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", PTR_ERR(bio));
+		err = PTR_ERR(bio);
+		rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", err);
 		goto sess_dev_put;
 	}
 
-- 
2.28.0

Re: [PATCH] block/rnbd: Ensure err is always initialized in process_rdma

[Jinpu Wang]

On Tue, Aug 18, 2020 at 8:50 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns:
>
> drivers/block/rnbd/rnbd-srv.c:150:6: warning: variable 'err' is used
> uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
>         if (IS_ERR(bio)) {
>             ^~~~~~~~~~~
> drivers/block/rnbd/rnbd-srv.c:177:9: note: uninitialized use occurs here
>         return err;
>                ^~~
> drivers/block/rnbd/rnbd-srv.c:150:2: note: remove the 'if' if its
> condition is always false
>         if (IS_ERR(bio)) {
>         ^~~~~~~~~~~~~~~~~~
> drivers/block/rnbd/rnbd-srv.c:126:9: note: initialize the variable 'err'
> to silence this warning
>         int err;
>                ^
>                 = 0
> 1 warning generated.
>
> err is indeed uninitialized when this statement is taken. Ensure that it
> is assigned the error value of bio before jumping to the error handling
> label.
>
> Fixes: 735d77d4fd28 ("rnbd: remove rnbd_dev_submit_io")
> Link: https://github.com/ClangBuiltLinux/linux/issues/1134
> Reported-by: Brooke Basile <brookebasile@gmail.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Thanks!
Acked-by: Jack Wang <jinpu.wang@cloud.ionos.com>
> ---
>  drivers/block/rnbd/rnbd-srv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> index 0fb94843a495..1b71cb2a885d 100644
> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -148,7 +148,8 @@ static int process_rdma(struct rtrs_srv *sess,
>         /* Generate bio with pages pointing to the rdma buffer */
>         bio = rnbd_bio_map_kern(data, sess_dev->rnbd_dev->ibd_bio_set, datalen, GFP_KERNEL);
>         if (IS_ERR(bio)) {
> -               rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", PTR_ERR(bio));
> +               err = PTR_ERR(bio);
> +               rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", err);
>                 goto sess_dev_put;
>         }
>
> --
> 2.28.0
>

Re: [PATCH] block: rnbd: rnbd-srv: silence uninitialized variable warning

[Brooke Basile]

On 8/18/20 1:29 AM, Nathan Chancellor wrote:
> I don't think this is a proper fix since the root cause of the warning
> appears to be that we are ignoring the return value of
> rnbd_bio_map_kern. Should we not set err to that value like this
> (completely untested)?
> 
> Cheers,
> Nathan
> 
> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> index 0fb94843a495..1b71cb2a885d 100644
> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -148,7 +148,8 @@ static int process_rdma(struct rtrs_srv *sess,
>   	/* Generate bio with pages pointing to the rdma buffer */
>   	bio = rnbd_bio_map_kern(data, sess_dev->rnbd_dev->ibd_bio_set, datalen, GFP_KERNEL);
>   	if (IS_ERR(bio)) {
> -		rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", PTR_ERR(bio));
> +		err = PTR_ERR(bio);
> +		rnbd_srv_err(sess_dev, "Failed to generate bio, err: %ld\n", err);
>   		goto sess_dev_put;
>   	}
>   
> 

Ah, I see what you mean.  Thanks for the fix!

Best,
Brooke Basile

Re: [PATCH] block/rnbd: Ensure err is always initialized in process_rdma

[Jens Axboe]

On 8/17/20 11:49 PM, Nathan Chancellor wrote:
> Clang warns:
> 
> drivers/block/rnbd/rnbd-srv.c:150:6: warning: variable 'err' is used
> uninitialized whenever 'if' condition is true
> [-Wsometimes-uninitialized]
>         if (IS_ERR(bio)) {
>             ^~~~~~~~~~~
> drivers/block/rnbd/rnbd-srv.c:177:9: note: uninitialized use occurs here
>         return err;
>                ^~~
> drivers/block/rnbd/rnbd-srv.c:150:2: note: remove the 'if' if its
> condition is always false
>         if (IS_ERR(bio)) {
>         ^~~~~~~~~~~~~~~~~~
> drivers/block/rnbd/rnbd-srv.c:126:9: note: initialize the variable 'err'
> to silence this warning
>         int err;
>                ^
>                 = 0
> 1 warning generated.
> 
> err is indeed uninitialized when this statement is taken. Ensure that it
> is assigned the error value of bio before jumping to the error handling
> label.

Applied, thanks.

-- 
Jens Axboe