[PATCH for-6.2 0/3] msg_ring fixes

[PATCH for-6.2 0/3] msg_ring fixes

[Pavel Begunkov]

First two patches are msg_ring fixes. 3/3 can go 6.3

Pavel Begunkov (3):
  io_uring/msg_ring: fix flagging remote execution
  io_uring/msg_ring: fix remote queue to disabled ring
  io_uring/msg_ring: optimise with correct tw notify method

 io_uring/io_uring.c |  4 ++--
 io_uring/msg_ring.c | 51 ++++++++++++++++++++++++++++++---------------
 2 files changed, 36 insertions(+), 19 deletions(-)

-- 
2.38.1

[PATCH for-6.2 1/3] io_uring/msg_ring: fix flagging remote execution

[Pavel Begunkov]

There is a couple of problems with queueing a tw in io_msg_ring_data()
for remote execution. First, once we queue it the target ring can
go away and so setting IORING_SQ_TASKRUN there is not safe. Secondly,
the userspace might not expect IORING_SQ_TASKRUN.

Extract a helper and uniformly use TWA_SIGNAL without TWA_SIGNAL_NO_IPI
tricks for now, just as it was done in the original patch.

Fixes: 6d043ee1164ca ("io_uring: do msg_ring in target task via tw")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/msg_ring.c | 40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index a333781565d3..bb868447dcdf 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -58,6 +58,25 @@ void io_msg_ring_cleanup(struct io_kiocb *req)
 	msg->src_file = NULL;
 }
 
+static inline bool io_msg_need_remote(struct io_ring_ctx *target_ctx)
+{
+	if (!target_ctx->task_complete)
+		return false;
+	return current != target_ctx->submitter_task;
+}
+
+static int io_msg_exec_remote(struct io_kiocb *req, task_work_func_t func)
+{
+	struct io_ring_ctx *ctx = req->file->private_data;
+	struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
+
+	init_task_work(&msg->tw, func);
+	if (task_work_add(ctx->submitter_task, &msg->tw, TWA_SIGNAL))
+		return -EOWNERDEAD;
+
+	return IOU_ISSUE_SKIP_COMPLETE;
+}
+
 static void io_msg_tw_complete(struct callback_head *head)
 {
 	struct io_msg *msg = container_of(head, struct io_msg, tw);
@@ -96,15 +115,8 @@ static int io_msg_ring_data(struct io_kiocb *req, unsigned int issue_flags)
 	if (msg->src_fd || msg->dst_fd || msg->flags)
 		return -EINVAL;
 
-	if (target_ctx->task_complete && current != target_ctx->submitter_task) {
-		init_task_work(&msg->tw, io_msg_tw_complete);
-		if (task_work_add(target_ctx->submitter_task, &msg->tw,
-				  TWA_SIGNAL_NO_IPI))
-			return -EOWNERDEAD;
-
-		atomic_or(IORING_SQ_TASKRUN, &target_ctx->rings->sq_flags);
-		return IOU_ISSUE_SKIP_COMPLETE;
-	}
+	if (io_msg_need_remote(target_ctx))
+		return io_msg_exec_remote(req, io_msg_tw_complete);
 
 	ret = -EOVERFLOW;
 	if (target_ctx->flags & IORING_SETUP_IOPOLL) {
@@ -202,14 +214,8 @@ static int io_msg_send_fd(struct io_kiocb *req, unsigned int issue_flags)
 		req->flags |= REQ_F_NEED_CLEANUP;
 	}
 
-	if (target_ctx->task_complete && current != target_ctx->submitter_task) {
-		init_task_work(&msg->tw, io_msg_tw_fd_complete);
-		if (task_work_add(target_ctx->submitter_task, &msg->tw,
-				  TWA_SIGNAL))
-			return -EOWNERDEAD;
-
-		return IOU_ISSUE_SKIP_COMPLETE;
-	}
+	if (io_msg_need_remote(target_ctx))
+		return io_msg_exec_remote(req, io_msg_tw_fd_complete);
 	return io_msg_install_complete(req, issue_flags);
 }
 
-- 
2.38.1

[PATCH for-6.2 2/3] io_uring/msg_ring: fix remote queue to disabled ring

[Pavel Begunkov]

IORING_SETUP_R_DISABLED rings don't have the submitter task set, so
it's not always safe to use ->submitter_task and we have to check if
it has already been set.

Fixes: 6d043ee1164ca ("io_uring: do msg_ring in target task via tw")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/io_uring.c | 4 ++--
 io_uring/msg_ring.c | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/io_uring/io_uring.c b/io_uring/io_uring.c
index 2ac1cd8d23ea..0a4efada9b3c 100644
--- a/io_uring/io_uring.c
+++ b/io_uring/io_uring.c
@@ -3674,7 +3674,7 @@ static __cold int io_uring_create(unsigned entries, struct io_uring_params *p,
 
 	if (ctx->flags & IORING_SETUP_SINGLE_ISSUER
 	    && !(ctx->flags & IORING_SETUP_R_DISABLED))
-		ctx->submitter_task = get_task_struct(current);
+		WRITE_ONCE(ctx->submitter_task, get_task_struct(current));
 
 	file = io_uring_get_file(ctx);
 	if (IS_ERR(file)) {
@@ -3868,7 +3868,7 @@ static int io_register_enable_rings(struct io_ring_ctx *ctx)
 		return -EBADFD;
 
 	if (ctx->flags & IORING_SETUP_SINGLE_ISSUER && !ctx->submitter_task)
-		ctx->submitter_task = get_task_struct(current);
+		WRITE_ONCE(ctx->submitter_task, get_task_struct(current));
 
 	if (ctx->restrictions.registered)
 		ctx->restricted = 1;
diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index bb868447dcdf..c68cd3898035 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -69,6 +69,10 @@ static int io_msg_exec_remote(struct io_kiocb *req, task_work_func_t func)
 {
 	struct io_ring_ctx *ctx = req->file->private_data;
 	struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
+	struct task_struct *task = READ_ONCE(ctx->submitter_task);
+
+	if (unlikely(!task))
+		return -EOWNERDEAD;
 
 	init_task_work(&msg->tw, func);
 	if (task_work_add(ctx->submitter_task, &msg->tw, TWA_SIGNAL))
-- 
2.38.1

[PATCH for-6.2 3/3] io_uring/msg_ring: optimise with correct tw notify method

[Pavel Begunkov]

We may want to use TWA_SIGNAL_NO_IPI instead of TWA_SIGNAL if the target
ring is configured with IORING_SETUP_COOP_TASKRUN, change
io_msg_exec_remote() to use the target ring's ->notify_method.

The caveat is that we have to set IORING_SQ_TASKRUN if the rings asks
for it. However, once task_work_add() succeeds the target ring might go
away and so we grab a ctx reference to pin the ring until we set
IORING_SQ_TASKRUN.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 io_uring/msg_ring.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index c68cd3898035..12dc9ed3d062 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -70,15 +70,22 @@ static int io_msg_exec_remote(struct io_kiocb *req, task_work_func_t func)
 	struct io_ring_ctx *ctx = req->file->private_data;
 	struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
 	struct task_struct *task = READ_ONCE(ctx->submitter_task);
+	int ret = IOU_ISSUE_SKIP_COMPLETE;
 
 	if (unlikely(!task))
 		return -EOWNERDEAD;
 
+	percpu_ref_get(&ctx->refs);
 	init_task_work(&msg->tw, func);
-	if (task_work_add(ctx->submitter_task, &msg->tw, TWA_SIGNAL))
-		return -EOWNERDEAD;
-
-	return IOU_ISSUE_SKIP_COMPLETE;
+	if (task_work_add(ctx->submitter_task, &msg->tw, ctx->notify_method)) {
+		ret = -EOWNERDEAD;
+		goto out;
+	}
+	if (ctx->flags & IORING_SETUP_TASKRUN_FLAG)
+		atomic_or(IORING_SQ_TASKRUN, &ctx->rings->sq_flags);
+out:
+	percpu_ref_put(&ctx->refs);
+	return ret;
 }
 
 static void io_msg_tw_complete(struct callback_head *head)
-- 
2.38.1

Re: [PATCH for-6.2 2/3] io_uring/msg_ring: fix remote queue to disabled ring

[Jens Axboe]

On 1/20/23 9:21 AM, Pavel Begunkov wrote:
> IORING_SETUP_R_DISABLED rings don't have the submitter task set, so
> it's not always safe to use ->submitter_task and we have to check if
> it has already been set.

As per private discussion, can we just forbid it in general?

-- 
Jens Axboe