Re: [PATCH v4 2/3] io_uring: Pass whole sqe to commands

[Pavel Begunkov <asml.silence@gmail.com>]

On 5/4/23 13:18, Breno Leitao wrote:
> Currently uring CMD operation relies on having large SQEs, but future
> operations might want to use normal SQE.
> 
> The io_uring_cmd currently only saves the payload (cmd) part of the SQE,
> but, for commands that use normal SQE size, it might be necessary to
> access the initial SQE fields outside of the payload/cmd block.  So,
> saves the whole SQE other than just the pdu.
> 
> This changes slightly how the io_uring_cmd works, since the cmd
> structures and callbacks are not opaque to io_uring anymore. I.e, the
> callbacks can look at the SQE entries, not only, in the cmd structure.
> 
> The main advantage is that we don't need to create custom structures for
> simple commands.
> 
> Creates io_uring_sqe_cmd() that returns the cmd private data as a null
> pointer and avoids casting in the callee side.
> Also, make most of ublk_drv's sqe->cmd priv structure into const, and use
> io_uring_sqe_cmd() to get the private structure, removing the unwanted
> cast. (There is one case where the cast is still needed since the
> header->{len,addr} is updated in the private structure)
> 
> Suggested-by: Pavel Begunkov <asml.silence@gmail.com>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Reviewed-by: Keith Busch <kbusch@kernel.org>
> ---
>   drivers/block/ublk_drv.c  | 26 +++++++++++++-------------
>   drivers/nvme/host/ioctl.c |  2 +-
>   include/linux/io_uring.h  |  7 ++++++-
>   io_uring/opdef.c          |  2 +-
>   io_uring/uring_cmd.c      |  9 +++------
>   5 files changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index c73cc57ec547..42f4d7ca962e 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
[...]
> @@ -2025,7 +2025,7 @@ static int ublk_ctrl_start_recovery(struct ublk_device *ub,
>   static int ublk_ctrl_end_recovery(struct ublk_device *ub,
>   		struct io_uring_cmd *cmd)
>   {
> -	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
> +	const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
>   	int ublksrv_pid = (int)header->data[0];
>   	int ret = -EINVAL;
>   
> @@ -2092,7 +2092,7 @@ static int ublk_char_dev_permission(struct ublk_device *ub,
>   static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
>   		struct io_uring_cmd *cmd)
>   {
> -	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
> +	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)io_uring_sqe_cmd(cmd->sqe);

Seems it's here to cast out const. Not an issue as apparently ctrl
goes to io-wq and so copies sqes, and it's definitely not a problem
of this patch, but seems fragile.


>   	bool unprivileged = ub->dev_info.flags & UBLK_F_UNPRIVILEGED_DEV;
>   	void __user *argp = (void __user *)(unsigned long)header->addr;
>   	char *dev_path = NULL;
> @@ -2171,7 +2171,7 @@ static int ublk_ctrl_uring_cmd_permission(struct ublk_device *ub,
>   static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
>   		unsigned int issue_flags)
>   {
> -	struct ublksrv_ctrl_cmd *header = (struct ublksrv_ctrl_cmd *)cmd->cmd;
> +	const struct ublksrv_ctrl_cmd *header = io_uring_sqe_cmd(cmd->sqe);
>   	struct ublk_device *ub = NULL;
>   	int ret = -EINVAL;
>   
[...]

-- 
Pavel Begunkov