Hi,
> -----Original Message-----
> From: Jens Axboe <axboe@xxxxxxxxx>
> Sent: Friday, February 14, 2025 12:31 AM
> To: io-uring <io-uring@xxxxxxxxxxxxxxx>
> Cc: Caleb Sander Mateos <csander@xxxxxxxxxxxxxxx>
> Subject: [PATCH v2] io_uring/uring_cmd: unconditionally copy SQEs at prep
> time
>
> This isn't generally necessary, but conditions have been observed where SQE
> data is accessed from the original SQE after prep has been done and outside of
> the initial issue. Opcode prep handlers must ensure that any SQE related data is
> stable beyond the prep phase, but uring_cmd is a bit special in how it handles
> the SQE which makes it susceptible to reading stale data. If the application has
> reused the SQE before the original completes, then that can lead to data
> corruption.
>
> Down the line we can relax this again once uring_cmd has been sanitized a bit,
> and avoid unnecessarily copying the SQE.
>
> Reported-by: Caleb Sander Mateos <csander@xxxxxxxxxxxxxxx>
> Reviewed-by: Caleb Sander Mateos <csander@xxxxxxxxxxxxxxx>
> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
>
> ---
>
> V2:
> - Pass in SQE for copy, and drop helper for copy
>
> diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index
> 8af7780407b7..e6701b7aa147 100644
> --- a/io_uring/uring_cmd.c
> +++ b/io_uring/uring_cmd.c
> @@ -165,15 +165,6 @@ void io_uring_cmd_done(struct io_uring_cmd
> *ioucmd, ssize_t ret, u64 res2, } EXPORT_SYMBOL_GPL(io_uring_cmd_done);
>
> -static void io_uring_cmd_cache_sqes(struct io_kiocb *req) -{
> - struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct
> io_uring_cmd);
> - struct io_uring_cmd_data *cache = req->async_data;
> -
> - memcpy(cache->sqes, ioucmd->sqe, uring_sqe_size(req->ctx));
> - ioucmd->sqe = cache->sqes;
> -}
> -
> static int io_uring_cmd_prep_setup(struct io_kiocb *req,
> const struct io_uring_sqe *sqe)
> {
> @@ -185,10 +176,15 @@ static int io_uring_cmd_prep_setup(struct io_kiocb
> *req,
> return -ENOMEM;
> cache->op_data = NULL;
>
> - ioucmd->sqe = sqe;
> - /* defer memcpy until we need it */
> - if (unlikely(req->flags & REQ_F_FORCE_ASYNC))
> - io_uring_cmd_cache_sqes(req);
> + /*
> + * Unconditionally cache the SQE for now - this is only needed for
> + * requests that go async, but prep handlers must ensure that any
> + * sqe data is stable beyond prep. Since uring_cmd is special in
> + * that it doesn't read in per-op data, play it safe and ensure that
> + * any SQE data is stable beyond prep. This can later get relaxed.
> + */
> + memcpy(cache->sqes, sqe, uring_sqe_size(req->ctx));
> + ioucmd->sqe = cache->sqes;
> return 0;
> }
>
> @@ -251,16 +247,8 @@ int io_uring_cmd(struct io_kiocb *req, unsigned int
> issue_flags)
> }
>
> ret = file->f_op->uring_cmd(ioucmd, issue_flags);
> - if (ret == -EAGAIN) {
> - struct io_uring_cmd_data *cache = req->async_data;
> -
> - if (ioucmd->sqe != cache->sqes)
> - io_uring_cmd_cache_sqes(req);
> - return -EAGAIN;
> - } else if (ret == -EIOCBQUEUED) {
> - return -EIOCBQUEUED;
> - }
> -
> + if (ret == -EAGAIN || ret == -EIOCBQUEUED)
> + return ret;
> if (ret < 0)
> req_set_fail(req);
> io_req_uring_cleanup(req, issue_flags);
>
> --
> Jens Axboe
>
>
Reviewed-by: Li Zetao <lizetao1@xxxxxxxxxx>
---
Li Zetao
