| 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
| ||
|
Message-ID: <CADUfDZovEN1MouTGyWHC4ZuhuPPTZ6WCkrS=yqa18xuJifuvqw@mail.gmail.com>
Date: Mon, 1 Sep 2025 18:42:31 -0700
From: Caleb Sander Mateos <csander@...estorage.com>
To: Ming Lei <ming.lei@...hat.com>, Jens Axboe <axboe@...nel.dk>
Cc: linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ublk: inline __ublk_ch_uring_cmd()
On Fri, Aug 8, 2025 at 8:32 AM Caleb Sander Mateos
<csander@...estorage.com> wrote:
>
> ublk_ch_uring_cmd_local() is a thin wrapper around __ublk_ch_uring_cmd()
> that copies the ublksrv_io_cmd from user-mapped memory to the stack
> using READ_ONCE(). This ublksrv_io_cmd is passed by pointer to
> __ublk_ch_uring_cmd() and __ublk_ch_uring_cmd() is a large function
> unlikely to be inlined, so __ublk_ch_uring_cmd() will have to load the
> ublksrv_io_cmd fields back from the stack. Inline __ublk_ch_uring_cmd()
> into ublk_ch_uring_cmd_local() and load the ublksrv_io_cmd fields into
> local variables with READ_ONCE(). This allows the compiler to delay
> loading the fields until they are needed and choose whether to store
> them in registers or on the stack.
Ming, thoughts on this patch? Do you see any value I'm missing in
keeping ublk_ch_uring_cmd_local() and __ublk_ch_uring_cmd() as
separate functions?
Thanks,
Caleb
>
> Signed-off-by: Caleb Sander Mateos <csander@...estorage.com>
> ---
> drivers/block/ublk_drv.c | 62 +++++++++++++++-------------------------
> 1 file changed, 23 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
> index 6561d2a561fa..a0ac944ec965 100644
> --- a/drivers/block/ublk_drv.c
> +++ b/drivers/block/ublk_drv.c
> @@ -2267,56 +2267,60 @@ static bool ublk_get_data(const struct ublk_queue *ubq, struct ublk_io *io,
> ublk_get_iod(ubq, req->tag)->addr);
>
> return ublk_start_io(ubq, req, io);
> }
>
> -static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> - unsigned int issue_flags,
> - const struct ublksrv_io_cmd *ub_cmd)
> +static int ublk_ch_uring_cmd_local(struct io_uring_cmd *cmd,
> + unsigned int issue_flags)
> {
> + /* May point to userspace-mapped memory */
> + const struct ublksrv_io_cmd *ub_src = io_uring_sqe_cmd(cmd->sqe);
> u16 buf_idx = UBLK_INVALID_BUF_IDX;
> struct ublk_device *ub = cmd->file->private_data;
> struct ublk_queue *ubq;
> struct ublk_io *io;
> u32 cmd_op = cmd->cmd_op;
> - unsigned tag = ub_cmd->tag;
> + u16 q_id = READ_ONCE(ub_src->q_id);
> + u16 tag = READ_ONCE(ub_src->tag);
> + s32 result = READ_ONCE(ub_src->result);
> + u64 addr = READ_ONCE(ub_src->addr); /* unioned with zone_append_lba */
> struct request *req;
> int ret;
> bool compl;
>
> + WARN_ON_ONCE(issue_flags & IO_URING_F_UNLOCKED);
> +
> pr_devel("%s: received: cmd op %d queue %d tag %d result %d\n",
> - __func__, cmd->cmd_op, ub_cmd->q_id, tag,
> - ub_cmd->result);
> + __func__, cmd->cmd_op, q_id, tag, result);
>
> 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 ublk_unregister_io_buf(cmd, ub, ub_cmd->addr,
> - issue_flags);
> + return ublk_unregister_io_buf(cmd, ub, addr, issue_flags);
>
> ret = -EINVAL;
> - if (ub_cmd->q_id >= ub->dev_info.nr_hw_queues)
> + if (q_id >= ub->dev_info.nr_hw_queues)
> goto out;
>
> - ubq = ublk_get_queue(ub, ub_cmd->q_id);
> + ubq = ublk_get_queue(ub, q_id);
>
> if (tag >= ubq->q_depth)
> goto out;
>
> io = &ubq->ios[tag];
> /* UBLK_IO_FETCH_REQ can be handled on any task, which sets io->task */
> if (unlikely(_IOC_NR(cmd_op) == UBLK_IO_FETCH_REQ)) {
> - ret = ublk_check_fetch_buf(ubq, ub_cmd->addr);
> + ret = ublk_check_fetch_buf(ubq, addr);
> if (ret)
> goto out;
> - ret = ublk_fetch(cmd, ubq, io, ub_cmd->addr);
> + ret = ublk_fetch(cmd, ubq, io, addr);
> if (ret)
> goto out;
>
> ublk_prep_cancel(cmd, issue_flags, ubq, tag);
> return -EIOCBQUEUED;
> @@ -2326,11 +2330,11 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> /*
> * ublk_register_io_buf() accesses only the io's refcount,
> * so can be handled on any task
> */
> if (_IOC_NR(cmd_op) == UBLK_IO_REGISTER_IO_BUF)
> - return ublk_register_io_buf(cmd, ubq, io, ub_cmd->addr,
> + return ublk_register_io_buf(cmd, ubq, io, addr,
> issue_flags);
>
> goto out;
> }
>
> @@ -2348,26 +2352,26 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> ^ (_IOC_NR(cmd_op) == UBLK_IO_NEED_GET_DATA))
> goto out;
>
> switch (_IOC_NR(cmd_op)) {
> case UBLK_IO_REGISTER_IO_BUF:
> - return ublk_daemon_register_io_buf(cmd, ubq, io, ub_cmd->addr,
> + return ublk_daemon_register_io_buf(cmd, ubq, io, addr,
> issue_flags);
> case UBLK_IO_COMMIT_AND_FETCH_REQ:
> - ret = ublk_check_commit_and_fetch(ubq, io, ub_cmd->addr);
> + ret = ublk_check_commit_and_fetch(ubq, io, addr);
> if (ret)
> goto out;
> - io->res = ub_cmd->result;
> + io->res = result;
> req = ublk_fill_io_cmd(io, cmd);
> - ret = ublk_config_io_buf(ubq, io, cmd, ub_cmd->addr, &buf_idx);
> + ret = ublk_config_io_buf(ubq, io, cmd, addr, &buf_idx);
> compl = ublk_need_complete_req(ubq, io);
>
> /* can't touch 'ublk_io' any more */
> if (buf_idx != UBLK_INVALID_BUF_IDX)
> io_buffer_unregister_bvec(cmd, buf_idx, issue_flags);
> if (req_op(req) == REQ_OP_ZONE_APPEND)
> - req->__sector = ub_cmd->zone_append_lba;
> + req->__sector = addr;
> if (compl)
> __ublk_complete_rq(req);
>
> if (ret)
> goto out;
> @@ -2377,11 +2381,11 @@ static int __ublk_ch_uring_cmd(struct io_uring_cmd *cmd,
> * ublk_get_data() may fail and fallback to requeue, so keep
> * uring_cmd active first and prepare for handling new requeued
> * request
> */
> req = ublk_fill_io_cmd(io, cmd);
> - ret = ublk_config_io_buf(ubq, io, cmd, ub_cmd->addr, NULL);
> + ret = ublk_config_io_buf(ubq, io, cmd, addr, NULL);
> WARN_ON_ONCE(ret);
> if (likely(ublk_get_data(ubq, io, req))) {
> __ublk_prep_compl_io_cmd(io, req);
> return UBLK_IO_RES_OK;
> }
> @@ -2428,30 +2432,10 @@ static inline struct request *__ublk_check_and_get_req(struct ublk_device *ub,
> fail_put:
> ublk_put_req_ref(io, req);
> return NULL;
> }
>
> -static inline int ublk_ch_uring_cmd_local(struct io_uring_cmd *cmd,
> - unsigned int issue_flags)
> -{
> - /*
> - * Not necessary for async retry, but let's keep it simple and always
> - * copy the values to avoid any potential reuse.
> - */
> - const struct ublksrv_io_cmd *ub_src = io_uring_sqe_cmd(cmd->sqe);
> - const struct ublksrv_io_cmd ub_cmd = {
> - .q_id = READ_ONCE(ub_src->q_id),
> - .tag = READ_ONCE(ub_src->tag),
> - .result = READ_ONCE(ub_src->result),
> - .addr = READ_ONCE(ub_src->addr)
> - };
> -
> - WARN_ON_ONCE(issue_flags & IO_URING_F_UNLOCKED);
> -
> - return __ublk_ch_uring_cmd(cmd, issue_flags, &ub_cmd);
> -}
> -
> static void ublk_ch_uring_cmd_cb(struct io_uring_cmd *cmd,
> unsigned int issue_flags)
> {
> int ret = ublk_ch_uring_cmd_local(cmd, issue_flags);
>
> --
> 2.45.2
>
Powered by blists - more mailing lists