On Fri, Jun 06, 2025 at 03:40:09PM -0600, Caleb Sander Mateos wrote:
> Currently, UBLK_IO_REGISTER_IO_BUF and UBLK_IO_UNREGISTER_IO_BUF are
> only permitted on the ublk_io's daemon task. But this restriction is
> unnecessary. ublk_register_io_buf() calls __ublk_check_and_get_req() to
> look up the request from the tagset and atomically take a reference on
> the request without accessing the ublk_io. ublk_unregister_io_buf()
> doesn't use the q_id or tag at all.
>
> So allow these opcodes even on tasks other than io->task.
>
> Handle UBLK_IO_UNREGISTER_IO_BUF before obtaining the ubq and io since
> the buffer index being unregistered is not necessarily related to the
> specified q_id and tag.
>
> Add a feature flag UBLK_F_BUF_REG_OFF_DAEMON that userspace can use to
> determine whether the kernel supports off-daemon buffer registration.
>
> Suggested-by: Ming Lei <ming.lei@xxxxxxxxxx>
> Signed-off-by: Caleb Sander Mateos <csander@xxxxxxxxxxxxxxx>
> ---
> drivers/block/ublk_drv.c | 37 +++++++++++++++++++++--------------
> include/uapi/linux/ublk_cmd.h | 8 ++++++++
> 2 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index a8030818f74a..2084bbdd2cbb 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -68,11 +68,12 @@
> | UBLK_F_ZONED \
> | UBLK_F_USER_RECOVERY_FAIL_IO \
> | UBLK_F_UPDATE_SIZE \
> | UBLK_F_AUTO_BUF_REG \
> | UBLK_F_QUIESCE \
> - | UBLK_F_PER_IO_DAEMON)
> + | UBLK_F_PER_IO_DAEMON \
> + | UBLK_F_BUF_REG_OFF_DAEMON)
>
> #define UBLK_F_ALL_RECOVERY_FLAGS (UBLK_F_USER_RECOVERY \
> | UBLK_F_USER_RECOVERY_REISSUE \
> | UBLK_F_USER_RECOVERY_FAIL_IO)
>
> @@ -2018,20 +2019,10 @@ static int ublk_register_io_buf(struct io_uring_cmd *cmd,
> }
>
> return 0;
> }
>
> -static int ublk_unregister_io_buf(struct io_uring_cmd *cmd,
> - const struct ublk_queue *ubq,
> - unsigned int index, unsigned int issue_flags)
> -{
> - if (!ublk_support_zero_copy(ubq))
> - return -EINVAL;
> -
> - return io_buffer_unregister_bvec(cmd, index, issue_flags);
> -}
> -
> static int ublk_fetch(struct io_uring_cmd *cmd, struct ublk_queue *ubq,
> struct ublk_io *io, __u64 buf_addr)
> {
> struct ublk_device *ub = ubq->dev;
> int ret = 0;
> @@ -2184,10 +2175,18 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
>
> ret = ublk_check_cmd_op(cmd_op);
> if (ret)
> goto out;
>
> + /*
> + * io_buffer_unregister_bvec() doesn't access the ubq or io,
> + * so no need to validate the q_id, tag, or task
> + */
> + if (_IOC_NR(cmd_op) == UBLK_IO_UNREGISTER_IO_BUF)
> + return io_buffer_unregister_bvec(cmd, ub_cmd->addr,
> + issue_flags);
> +
Yeah, the behavior looks correct, but I'd suggest to validate the q_id
too for making code more robust.
Also you removed ublk_support_zero_copy() check for unregistering io buffer
command, which isn't expected for this patch.
> ret = -EINVAL;
> if (ub_cmd->q_id >= ub->dev_info.nr_hw_queues)
> goto out;
>
> ubq = ublk_get_queue(ub, ub_cmd->q_id);
> @@ -2204,12 +2203,21 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
>
> ublk_prep_cancel(cmd, issue_flags, ubq, tag);
> return -EIOCBQUEUED;
> }
>
> - if (READ_ONCE(io->task) != current)
> + if (READ_ONCE(io->task) != current) {
> + /*
> + * ublk_register_io_buf() accesses only the request, not io,
> + * so can be handled on any task
> + */
> + if (_IOC_NR(cmd_op) == UBLK_IO_REGISTER_IO_BUF)
> + return ublk_register_io_buf(cmd, ubq, tag, ub_cmd->addr,
> + issue_flags);
Maybe you can move UBLK_IO_UNREGISTER_IO_BUF handling here, which seems
more readable.
Thanks,
Ming
