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

[Ziyang Zhang <ZiyangZhang@xxxxxxxxxxxxxxxxx>]

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@xxxxxxxxx>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> ---
>  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;