RE: [PATCH v2] io_uring/uring_cmd: unconditionally copy SQEs at prep time

[lizetao <lizetao1@huawei.com>]

Hi,

> -----Original Message-----
> From: Jens Axboe <axboe@kernel.dk>
> Sent: Friday, February 14, 2025 12:31 AM
> To: io-uring <io-uring@vger.kernel.org>
> Cc: Caleb Sander Mateos <csander@purestorage.com>
> 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@purestorage.com>
> Reviewed-by: Caleb Sander Mateos <csander@purestorage.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> 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@huawei.com>

---
Li Zetao