Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b32d275b-bb7a-4090-1d13-945ffca63c91@gmail.com>
Date:   Thu, 4 May 2023 15:04:25 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v4 2/3] io_uring: Pass whole sqe to commands
Content-Language: en-US
To:     Breno Leitao <leitao@debian.org>, io-uring@vger.kernel.org,
        linux-nvme@lists.infradead.org, hch@lst.de, axboe@kernel.dk,
        ming.lei@redhat.com
Cc:     leit@fb.com, linux-kernel@vger.kernel.org,
        linux-block@vger.kernel.org, sagi@grimberg.me, joshi.k@samsung.com,
        kbusch@kernel.org
References: <20230504121856.904491-1-leitao@debian.org>
 <20230504121856.904491-3-leitao@debian.org>
From:   Pavel Begunkov <asml.silence@gmail.com>
In-Reply-To: <20230504121856.904491-3-leitao@debian.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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