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

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

[Jens Axboe]

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

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

[Caleb Sander Mateos]

On Thu, Feb 13, 2025 at 8:30 AM Jens Axboe <axboe@kernel.dk> wrote:
>
> 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

v2 looks good to me. You might add "Fixes: 5eff57fa9f3a", since we
know it fixes the potential SQE corruption in the link and drain
cases.

>
> 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
>

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

[lizetao]

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

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

[Jens Axboe]

On 2/13/25 9:39 AM, Caleb Sander Mateos wrote:
> On Thu, Feb 13, 2025 at 8:30?AM Jens Axboe <axboe@kernel.dk> wrote:
>>
>> 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
> 
> v2 looks good to me. You might add "Fixes: 5eff57fa9f3a", since we
> know it fixes the potential SQE corruption in the link and drain
> cases.

Sure, I'll add that, reduces the risk of it being missed for stable.

-- 
Jens Axboe