[PATCH] ublk_drv: fix build warning with -Wmaybe-uninitialized and one sparse warning

[PATCH] ublk_drv: fix build warning with -Wmaybe-uninitialized and one sparse warning

[Ming Lei]

After applying -Wmaybe-uninitialized manually, two build warnings are
triggered:

drivers/block/ublk_drv.c:940:11: warning: ‘io’ may be used uninitialized [-Wmaybe-uninitialized]
  940 |         io->flags &= ~UBLK_IO_FLAG_ACTIVE;

drivers/block/ublk_drv.c: In function ‘ublk_ctrl_uring_cmd’:
drivers/block/ublk_drv.c:1531:9: warning: ‘ret’ may be used uninitialized [-Wmaybe-uninitialized]

Fix the 1st one by removing 'io->flags &= ~UBLK_IO_FLAG_ACTIVE;' which
isn't needed since the function always return successfully after setting
this flag.

Fix the 2nd one by always initializing 'ret'.

Also fix another sparse warning of 'sparse: sparse: incorrect type in return
expression' by changing return type of ublk_setup_iod().

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/ublk_drv.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index f10c4319dc1f..2c1b01d7f27d 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -407,7 +407,7 @@ static inline unsigned int ublk_req_build_flags(struct request *req)
 	return flags;
 }
 
-static int ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
+static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
 {
 	struct ublksrv_io_desc *iod = ublk_get_iod(ubq, req->tag);
 	struct ublk_io *io = &ubq->ios[req->tag];
@@ -937,7 +937,6 @@ static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
 	return -EIOCBQUEUED;
 
  out:
-	io->flags &= ~UBLK_IO_FLAG_ACTIVE;
 	io_uring_cmd_done(cmd, ret, 0);
 	pr_devel("%s: complete: cmd op %d, tag %d ret %x io_flags %x\n",
 			__func__, cmd_op, tag, ret, io->flags);
@@ -1299,13 +1298,12 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
 	struct ublk_device *ub;
 	unsigned long queue;
 	unsigned int retlen;
-	int ret;
+	int ret = -EINVAL;
 
 	ub = ublk_get_device_from_id(header->dev_id);
 	if (!ub)
 		goto out;
 
-	ret = -EINVAL;
 	queue = header->data[0];
 	if (queue >= ub->dev_info.nr_hw_queues)
 		goto out;
-- 
2.36.1

Re: [PATCH] ublk_drv: fix build warning with -Wmaybe-uninitialized and one sparse warning

[Jens Axboe]

On Sat, 16 Jul 2022 17:53:44 +0800, Ming Lei wrote:
> After applying -Wmaybe-uninitialized manually, two build warnings are
> triggered:
> 
> drivers/block/ublk_drv.c:940:11: warning: ‘io’ may be used uninitialized [-Wmaybe-uninitialized]
>   940 |         io->flags &= ~UBLK_IO_FLAG_ACTIVE;
> 
> drivers/block/ublk_drv.c: In function ‘ublk_ctrl_uring_cmd’:
> drivers/block/ublk_drv.c:1531:9: warning: ‘ret’ may be used uninitialized [-Wmaybe-uninitialized]
> 
> [...]

Applied, thanks!

[1/1] ublk_drv: fix build warning with -Wmaybe-uninitialized and one sparse warning
      commit: f2450f8a2c1ec3e88d6674f747b913aa5f21fa59

Best regards,
-- 
Jens Axboe

Re: [PATCH] ublk_drv: fix build warning with -Wmaybe-uninitialized and one sparse warning

[Ziyang Zhang]

On 2022/7/16 17:53, Ming Lei wrote:
> After applying -Wmaybe-uninitialized manually, two build warnings are
> triggered:
> 
> drivers/block/ublk_drv.c:940:11: warning: ‘io’ may be used uninitialized [-Wmaybe-uninitialized]
>   940 |         io->flags &= ~UBLK_IO_FLAG_ACTIVE;
> 
> drivers/block/ublk_drv.c: In function ‘ublk_ctrl_uring_cmd’:
> drivers/block/ublk_drv.c:1531:9: warning: ‘ret’ may be used uninitialized [-Wmaybe-uninitialized]
> 
> Fix the 1st one by removing 'io->flags &= ~UBLK_IO_FLAG_ACTIVE;' which
> isn't needed since the function always return successfully after setting
> this flag.
> 
> Fix the 2nd one by always initializing 'ret'.
> 
> Also fix another sparse warning of 'sparse: sparse: incorrect type in return
> expression' by changing return type of ublk_setup_iod().
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
>  drivers/block/ublk_drv.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index f10c4319dc1f..2c1b01d7f27d 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -407,7 +407,7 @@ static inline unsigned int ublk_req_build_flags(struct request *req)
>  	return flags;
>  }
>  
> -static int ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
> +static blk_status_t ublk_setup_iod(struct ublk_queue *ubq, struct request *req)
>  {
>  	struct ublksrv_io_desc *iod = ublk_get_iod(ubq, req->tag);
>  	struct ublk_io *io = &ubq->ios[req->tag];
> @@ -937,7 +937,6 @@ static int ublk_ch_uring_cmd(struct io_uring_cmd *cmd, unsigned int issue_flags)
>  	return -EIOCBQUEUED;
>  
>   out:
> -	io->flags &= ~UBLK_IO_FLAG_ACTIVE;
>  	io_uring_cmd_done(cmd, ret, 0);

Hi Ming,

Actually I found one potential bug BEFORE this commit.

In ublk_ch_uring_cmd(), there is a check:

	/* there is pending io cmd, something must be wrong */
	if (io->flags & UBLK_IO_FLAG_ACTIVE) {
		ret = -EBUSY;
		goto out;
	}

Now assume that:

 (1) We get a second(duplicate) ublk cmd(such as UBLK_IO_COMMIT_AND_FETCH_REQ)
     on the same tag and queue from a malicious app,
     the check fails as expected because UBLK_IO_FLAG_ACTIVE has been set 
     and ublk_io is queued. 

 (2) We goto label "out" and execute: 
        io->flags &= ~UBLK_IO_FLAG_ACTIVE
           -->  "io_uring_cmd_done(cmd, ret, 0)(ret is -EBUSY)

 (2) Then, if the malicious app issues a third ublk cmd(the same cmd_op) on the same
     tag and queue, the check passes because UBLK_IO_FLAG_ACTIVE is unset
     (it should fail since it is a duplicate cmd)

In this commit you do fix it by removing "io->flags &= ~UBLK_IO_FLAG_ACTIVE"
in "out" routine for another reason.(‘io’ may be used uninitialized)

However I have just found a side effect(maybe fix a potential bug) and share it here. :)

>  	pr_devel("%s: complete: cmd op %d, tag %d ret %x io_flags %x\n",
>  			__func__, cmd_op, tag, ret, io->flags);
> @@ -1299,13 +1298,12 @@ static int ublk_ctrl_get_queue_affinity(struct io_uring_cmd *cmd)
>  	struct ublk_device *ub;
>  	unsigned long queue;
>  	unsigned int retlen;
> -	int ret;
> +	int ret = -EINVAL;
>  
>  	ub = ublk_get_device_from_id(header->dev_id);
>  	if (!ub)
>  		goto out;
>  
> -	ret = -EINVAL;
>  	queue = header->data[0];
>  	if (queue >= ub->dev_info.nr_hw_queues)
>  		goto out;