On Sat, Apr 26, 2025 at 2:41 AM Ming Lei <ming.lei@xxxxxxxxxx> 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.
Indeed, both these checks make sense. (Hopefully there aren't any
applications depending on the ability to use ublk zero-copy without
setting the flag.) I too was thinking of adding the
UBLK_IO_FLAG_OWNED_BY_SRV check because it could allow the
kref_get_unless_zero() to be replaced with the cheaper kref_get(). I
think the checks could be split into 2 separate commits, but up to
you.
>
> Fixes: 1f6540e2aabb ("ublk: zc register/unregister bvec")
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> ---
> drivers/block/ublk_drv.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 40f971a66d3e..347790b3a633 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -609,6 +609,11 @@ static void ublk_apply_params(struct ublk_device *ub)
> ublk_dev_param_zoned_apply(ub);
> }
>
> +static inline bool ublk_support_zero_copy(const struct ublk_queue *ubq)
> +{
> + return ubq->flags & UBLK_F_SUPPORT_ZERO_COPY;
> +}
> +
> static inline bool ublk_support_user_copy(const struct ublk_queue *ubq)
> {
> return ubq->flags & (UBLK_F_USER_COPY | UBLK_F_SUPPORT_ZERO_COPY);
> @@ -1950,9 +1955,16 @@ static int ublk_register_io_buf(struct io_uring_cmd *cmd,
> unsigned int index, unsigned int issue_flags)
> {
> struct ublk_device *ub = cmd->file->private_data;
> + struct ublk_io *io = &ubq->ios[tag];
I thought you had mentioned in
https://lore.kernel.org/linux-block/aAmYJxaV1-yWEMRo@fedora/ wanting
to the ability to offload the ublk zero-copy buffer registration to a
thread other than ubq_daemon. Are you still planning to do that, or
does the "auto-register" feature supplant the need for that? Accessing
the ublk_io here only seems safe when on the ubq_daemon thread.
> struct request *req;
> int ret;
>
> + if (!ublk_support_zero_copy(ubq))
> + return -EINVAL;
> +
> + if (!(io->flags & UBLK_IO_FLAG_OWNED_BY_SRV))
> + return -EINVAL;
Every opcode except UBLK_IO_FETCH_REQ now checks io->flags &
UBLK_IO_FLAG_OWNED_BY_SRV. Maybe it would make sense to lift the check
up to __ublk_ch_uring_cmd() to avoid duplicating it?
Best,
Caleb
> +
> req = __ublk_check_and_get_req(ub, ubq, tag, 0);
> if (!req)
> return -EINVAL;
> @@ -1968,8 +1980,17 @@ static int ublk_register_io_buf(struct io_uring_cmd *cmd,
> }
>
> static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
> + struct ublk_queue *ubq, unsigned int tag,
> unsigned int index, unsigned int issue_flags)
> {
> + 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);
> }
>
> @@ -2073,7 +2094,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)
> --
> 2.47.0
>
