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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 4 May 2023 15:04:25 +0100
From:   Pavel Begunkov <asml.silence@...il.com>
To:     Breno Leitao <leitao@...ian.org>, io-uring@...r.kernel.org,
        linux-nvme@...ts.infradead.org, hch@....de, axboe@...nel.dk,
        ming.lei@...hat.com
Cc:     leit@...com, linux-kernel@...r.kernel.org,
        linux-block@...r.kernel.org, sagi@...mberg.me, joshi.k@...sung.com,
        kbusch@...nel.org
Subject: Re: [PATCH v4 2/3] io_uring: Pass whole sqe to commands

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@...il.com>
> Signed-off-by: Breno Leitao <leitao@...ian.org>
> Reviewed-by: Keith Busch <kbusch@...nel.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