Re: [PATCH 6/8] ublk: allow UBLK_IO_(UN)REGISTER_IO_BUF on any task

[Ming Lei <ming.lei@redhat.com>]

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@redhat.com>
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
> ---
>  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