Re: [PATCH v6.15 3/3] ublk: enhance check for register/unregister io buffer command

Caleb Sander Mateos
2025-04-28T16:28:19Z

On Sun, Apr 27, 2025 at 6:50 AM Ming Lei <ming.lei@redhat.com> wrote:
>
> The simple check of UBLK_IO_FLAG_OWNED_BY_SRV can avoid incorrect
> register/unregister io buffer easily, so check it before calling
> starting to register/un-register io buffer.
>
> Also only allow io buffer register/unregister uring_cmd in case of
> UBLK_F_SUPPORT_ZERO_COPY.
>
> Also mark argument 'ublk_queue *' of ublk_register_io_buf as const.
>
> Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---
> drivers/block/ublk_drv.c | 24 ++++++++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 0a3a3c64316d..c624d8f653ae 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -201,7 +201,7 @@ struct ublk_params_header {
> static void ublk_stop_dev_unlocked(struct ublk_device *ub);
> static void ublk_abort_queue(struct ublk_device *ub, struct ublk_queue *ubq);
> static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> - struct ublk_queue *ubq, int tag, size_t offset);
> + const struct ublk_queue *ubq, int tag, size_t offset);
> static inline unsigned int ublk_req_build_flags(struct request *req);
> static inline struct ublksrv_io_desc *ublk_get_iod(struct ublk_queue *ubq,
> int tag);
> @@ -1949,13 +1949,20 @@ static void ublk_io_release(void *priv)
> }
>
> static int ublk_register_io_buf(struct io_uring_cmd *cmd,
> - struct ublk_queue *ubq, unsigned int tag,
> + const struct ublk_queue *ubq, unsigned int tag,
> unsigned int index, unsigned int issue_flags)
> {
> struct ublk_device *ub = cmd->file->private_data;
> + const struct ublk_io *io = &ubq->ios[tag];
> struct request *req;
> int ret;
>
> + if (!ublk_support_zero_copy(ubq))
> + return -EINVAL;
> +
> + if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
> + return -EINVAL;

I would still prefer to see this common UBLK_IO_FLAG_OWNED_BY_SRV
check moved to __ublk_ch_uring_cmd() along with the existing flag
checks. Something like this:

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index a183aa7648c3..79ef2580ddcc 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2055,6 +2055,11 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
goto out;
}

+ /* only FETCH_REQ is allowed if io is not OWNED_BY_SRV */
+ if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV) &&
+ _IOC_NR(cmd_op) != UBLK_IO_FETCH_REQ)
+ goto out;
+
/*
* ensure that the user issues UBLK_IO_NEED_GET_DATA
* iff the driver have set the UBLK_IO_FLAG_NEED_GET_DATA.
@@ -2080,10 +2085,6 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
break;
case UBLK_IO_COMMIT_AND_FETCH_REQ:
req = blk_mq_tag_to_rq(ub->tag_set.tags[ub_cmd->q_id], tag);
-
- if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
- goto out;
-
if (ublk_need_map_io(ubq)) {
/*
* COMMIT_AND_FETCH_REQ has to provide IO buffer if
@@ -2105,8 +2106,6 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
ublk_commit_completion(ub, ub_cmd);
break;
case UBLK_IO_NEED_GET_DATA:
- if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
- goto out;
ublk_fill_io_cmd(io, cmd, ub_cmd->addr);
req = blk_mq_tag_to_rq(ub->tag_set.tags[ub_cmd->q_id], tag);
ublk_dispatch_req(ubq, req, issue_flags);

> +
> req = __ublk_check_and_get_req(ub, ubq, tag, 0);
> if (!req)
> return -EINVAL;
> @@ -1971,8 +1978,17 @@ static int ublk_register_io_buf(struct io_uring_cmd *cmd,
> }
>
> static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
> + const struct ublk_queue *ubq, unsigned int tag,
> unsigned int index, unsigned int issue_flags)
> {
> + const struct ublk_io *io = &ubq->ios[tag];
> +
> + if (!ublk_support_zero_copy(ubq))
> + return -EINVAL;
> +
> + if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
> + return -EINVAL;
> +
> return io_buffer_unregister_bvec(cmd, index, issue_flags);
> }
>
> @@ -2076,7 +2092,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> case UBLK_IO_REGISTER_IO_BUF:
> return ublk_register_io_buf(cmd, ubq, tag, ub_cmd->addr, issue_flags);
> case UBLK_IO_UNREGISTER_IO_BUF:
> - return ublk_unregister_io_buf(cmd, ub_cmd->addr, issue_flags);
> + return ublk_unregister_io_buf(cmd, ubq, tag, ub_cmd->addr, issue_flags);
> case UBLK_IO_FETCH_REQ:
> ret = ublk_fetch(cmd, ubq, io, ub_cmd->addr);
> if (ret)
> @@ -2128,7 +2144,7 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> }
>
> static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> - struct ublk_queue *ubq, int tag, size_t offset)
> + const struct ublk_queue *ubq, int tag, size_t offset)
> {
> struct request *req;
>
> --
> 2.47.0
>