diff mbox series

[2/3] io_uring: Use submit info inlined into req

Message ID 32cc59cefc848ba2e258fc4581684f1c2e67d649.1572993994.git.asml.silence@gmail.com (mailing list archive)
State New, archived
Headers show
Series Inline sqe_submit | expand

Commit Message

Pavel Begunkov Nov. 5, 2019, 11:04 p.m. UTC
Stack allocated struct sqe_submit is passed down to the submission path
along with a request (a.k.a. struct io_kiocb), and will be copied into
req->submit for async requests.

As space for it is already allocated, fill req->submit in the first
place instead of using on-stack one. As a result:

1. sqe->submit is the only place for sqe_submit and is always valid,
so we don't need to track which one to use.
2. don't need to copy in case of async
3. allows to simplify the code by not carrying it as an argument all
the way down
4. allows to reduce number of function arguments / potentially improve
spilling

The downside is that stack is most probably be cached, that's not true
for just allocated memory for a request. Another concern is cache
pollution. Though, a request would be touched and fetched along with
req->submit at some point anyway, so shouldn't be a problem.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

Comments

Jens Axboe Nov. 5, 2019, 11:42 p.m. UTC | #1
On 11/5/19 4:04 PM, Pavel Begunkov wrote:
 				if (unlikely(!shadow_req))
> @@ -2716,24 +2712,25 @@ static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
>   				shadow_req->flags |= (REQ_F_IO_DRAIN | REQ_F_SHADOW_DRAIN);
>   				refcount_dec(&shadow_req->refs);
>   			}
> -			shadow_req->sequence = s.sequence;
> +			shadow_req->sequence = req->submit.sequence;
>   		}
>   
>   out:
> -		s.ring_file = ring_file;
> -		s.ring_fd = ring_fd;
> -		s.has_user = *mm != NULL;
> -		s.in_async = async;
> -		s.needs_fixed_file = async;
> -		trace_io_uring_submit_sqe(ctx, s.sqe->user_data, true, async);
> -		io_submit_sqe(ctx, req, &s, statep, &link);
> +		req->submit.ring_file = ring_file;
> +		req->submit.ring_fd = ring_fd;
> +		req->submit.has_user = *mm != NULL;
> +		req->submit.in_async = async;
> +		req->submit.needs_fixed_file = async;
> +		trace_io_uring_submit_sqe(ctx, req->submit.sqe->user_data,
> +					  true, async);
> +		io_submit_sqe(ctx, req, &req->submit, statep, &link);
>   		submitted++;
>   
>   		/*
>   		 * If previous wasn't linked and we have a linked command,
>   		 * that's the end of the chain. Submit the previous link.
>   		 */
> -		if (!(s.sqe->flags & IOSQE_IO_LINK) && link) {
> +		if (!(req->submit.sqe->flags & IOSQE_IO_LINK) && link) {
>   			io_queue_link_head(ctx, link, &link->submit, shadow_req);
>   			link = NULL;
>   			shadow_req = NULL;

Another potential use-after-free here, as 'req' might have completed by
the time you go and check for IOSQE_IO_LINK.
diff mbox series

Patch

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 920ad731db01..ecb5a4336389 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2443,7 +2443,6 @@  static int __io_queue_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 		sqe_copy = kmemdup(s->sqe, sizeof(*sqe_copy), GFP_KERNEL);
 		if (sqe_copy) {
 			s->sqe = sqe_copy;
-			memcpy(&req->submit, s, sizeof(*s));
 			if (req->work.flags & IO_WQ_WORK_NEEDS_FILES) {
 				ret = io_grab_files(ctx, req);
 				if (ret) {
@@ -2578,13 +2577,11 @@  static void io_submit_sqe(struct io_ring_ctx *ctx, struct io_kiocb *req,
 		}
 
 		s->sqe = sqe_copy;
-		memcpy(&req->submit, s, sizeof(*s));
 		trace_io_uring_link(ctx, req, prev);
 		list_add_tail(&req->list, &prev->link_list);
 	} else if (s->sqe->flags & IOSQE_IO_LINK) {
 		req->flags |= REQ_F_LINK;
 
-		memcpy(&req->submit, s, sizeof(*s));
 		INIT_LIST_HEAD(&req->link_list);
 		*link = req;
 	} else {
@@ -2689,18 +2686,17 @@  static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
 	}
 
 	for (i = 0; i < nr; i++) {
-		struct sqe_submit s;
 		struct io_kiocb *req;
 
 		req = io_get_req(ctx, statep);
 		if (unlikely(!req))
 			break;
-		if (!io_get_sqring(ctx, &s)) {
+		if (!io_get_sqring(ctx, &req->submit)) {
 			__io_free_req(req);
 			break;
 		}
 
-		if (io_sqe_needs_user(s.sqe) && !*mm) {
+		if (io_sqe_needs_user(req->submit.sqe) && !*mm) {
 			mm_fault = mm_fault || !mmget_not_zero(ctx->sqo_mm);
 			if (!mm_fault) {
 				use_mm(ctx->sqo_mm);
@@ -2708,7 +2704,7 @@  static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
 			}
 		}
 
-		if (link && (s.sqe->flags & IOSQE_IO_DRAIN)) {
+		if (link && (req->submit.sqe->flags & IOSQE_IO_DRAIN)) {
 			if (!shadow_req) {
 				shadow_req = io_get_req(ctx, NULL);
 				if (unlikely(!shadow_req))
@@ -2716,24 +2712,25 @@  static int io_submit_sqes(struct io_ring_ctx *ctx, unsigned int nr,
 				shadow_req->flags |= (REQ_F_IO_DRAIN | REQ_F_SHADOW_DRAIN);
 				refcount_dec(&shadow_req->refs);
 			}
-			shadow_req->sequence = s.sequence;
+			shadow_req->sequence = req->submit.sequence;
 		}
 
 out:
-		s.ring_file = ring_file;
-		s.ring_fd = ring_fd;
-		s.has_user = *mm != NULL;
-		s.in_async = async;
-		s.needs_fixed_file = async;
-		trace_io_uring_submit_sqe(ctx, s.sqe->user_data, true, async);
-		io_submit_sqe(ctx, req, &s, statep, &link);
+		req->submit.ring_file = ring_file;
+		req->submit.ring_fd = ring_fd;
+		req->submit.has_user = *mm != NULL;
+		req->submit.in_async = async;
+		req->submit.needs_fixed_file = async;
+		trace_io_uring_submit_sqe(ctx, req->submit.sqe->user_data,
+					  true, async);
+		io_submit_sqe(ctx, req, &req->submit, statep, &link);
 		submitted++;
 
 		/*
 		 * If previous wasn't linked and we have a linked command,
 		 * that's the end of the chain. Submit the previous link.
 		 */
-		if (!(s.sqe->flags & IOSQE_IO_LINK) && link) {
+		if (!(req->submit.sqe->flags & IOSQE_IO_LINK) && link) {
 			io_queue_link_head(ctx, link, &link->submit, shadow_req);
 			link = NULL;
 			shadow_req = NULL;