* Re: [PATCH 0/2] Fix locking for MSG_RING and IOPOLL targets
2023-08-22 23:30 ` Jens Axboe
@ 2023-08-23 0:06 ` Jens Axboe
0 siblings, 0 replies; 7+ messages in thread
From: Jens Axboe @ 2023-08-23 0:06 UTC (permalink / raw)
To: Oleksandr Tymoshenko; +Cc: asml.silence, io-uring
[-- Attachment #1: Type: text/plain, Size: 324 bytes --]
On 8/22/23 5:30 PM, Jens Axboe wrote:
> On 8/22/23 2:54 PM, Oleksandr Tymoshenko wrote:
>> Hello,
>>
>> Does this patchset need to be backported to 6.1?
>
> Yeah it probably should, guessing it didn't get picked as there are
> dependencies. I'll take a look.
Here they are - I'll send them in for stable.
--
Jens Axboe
[-- Attachment #2: 0004-io_uring-msg_ring-fix-missing-lock-on-overflow-for-I.patch --]
[-- Type: text/x-patch, Size: 2379 bytes --]
From c47db9ca937e232606060077bb09cff49e75dbb7 Mon Sep 17 00:00:00 2001
From: Jens Axboe <[email protected]>
Date: Tue, 22 Aug 2023 18:00:02 -0600
Subject: [PATCH 4/4] io_uring/msg_ring: fix missing lock on overflow for
IOPOLL
Commit e12d7a46f65ae4b7d58a5e0c1cbfa825cf8d830d upstream.
If the target ring is configured with IOPOLL, then we always need to hold
the target ring uring_lock before posting CQEs. We could just grab it
unconditionally, but since we don't expect many target rings to be of this
type, make grabbing the uring_lock conditional on the ring type.
Link: https://lore.kernel.org/io-uring/Y8krlYa52%[email protected]/
Reported-by: Xingyuan Mo <[email protected]>
Signed-off-by: Jens Axboe <[email protected]>
---
io_uring/msg_ring.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index 825d8c579fd3..cd922d2bef5f 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -57,20 +57,30 @@ void io_msg_ring_cleanup(struct io_kiocb *req)
msg->src_file = NULL;
}
-static int io_msg_ring_data(struct io_kiocb *req)
+static int io_msg_ring_data(struct io_kiocb *req, unsigned int issue_flags)
{
struct io_ring_ctx *target_ctx = req->file->private_data;
struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
+ int ret;
if (msg->src_fd || msg->dst_fd || msg->flags)
return -EINVAL;
if (target_ctx->flags & IORING_SETUP_R_DISABLED)
return -EBADFD;
- if (io_post_aux_cqe(target_ctx, msg->user_data, msg->len, 0, true))
- return 0;
+ ret = -EOVERFLOW;
+ if (target_ctx->flags & IORING_SETUP_IOPOLL) {
+ if (unlikely(io_double_lock_ctx(target_ctx, issue_flags)))
+ return -EAGAIN;
+ if (io_post_aux_cqe(target_ctx, msg->user_data, msg->len, 0, true))
+ ret = 0;
+ io_double_unlock_ctx(target_ctx);
+ } else {
+ if (io_post_aux_cqe(target_ctx, msg->user_data, msg->len, 0, true))
+ ret = 0;
+ }
- return -EOVERFLOW;
+ return ret;
}
static struct file *io_msg_grab_file(struct io_kiocb *req, unsigned int issue_flags)
@@ -175,7 +185,7 @@ int io_msg_ring(struct io_kiocb *req, unsigned int issue_flags)
switch (msg->cmd) {
case IORING_MSG_DATA:
- ret = io_msg_ring_data(req);
+ ret = io_msg_ring_data(req, issue_flags);
break;
case IORING_MSG_SEND_FD:
ret = io_msg_send_fd(req, issue_flags);
--
2.40.1
[-- Attachment #3: 0003-io_uring-msg_ring-move-double-lock-unlock-helpers-hi.patch --]
[-- Type: text/x-patch, Size: 2648 bytes --]
From 5c3ffa8ec64e0726dcbfe37b5d126701301011ed Mon Sep 17 00:00:00 2001
From: Jens Axboe <[email protected]>
Date: Thu, 19 Jan 2023 09:01:27 -0700
Subject: [PATCH 3/4] io_uring/msg_ring: move double lock/unlock helpers higher
up
Commit 423d5081d0451faa59a707e57373801da5b40141 upstream.
In preparation for needing them somewhere else, move them and get rid of
the unused 'issue_flags' for the unlock side.
No functional changes in this patch.
Signed-off-by: Jens Axboe <[email protected]>
---
io_uring/msg_ring.c | 47 ++++++++++++++++++++++-----------------------
1 file changed, 23 insertions(+), 24 deletions(-)
diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index fd96a0cd85f0..825d8c579fd3 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -24,6 +24,28 @@ struct io_msg {
u32 flags;
};
+static void io_double_unlock_ctx(struct io_ring_ctx *octx)
+{
+ mutex_unlock(&octx->uring_lock);
+}
+
+static int io_double_lock_ctx(struct io_ring_ctx *octx,
+ unsigned int issue_flags)
+{
+ /*
+ * To ensure proper ordering between the two ctxs, we can only
+ * attempt a trylock on the target. If that fails and we already have
+ * the source ctx lock, punt to io-wq.
+ */
+ if (!(issue_flags & IO_URING_F_UNLOCKED)) {
+ if (!mutex_trylock(&octx->uring_lock))
+ return -EAGAIN;
+ return 0;
+ }
+ mutex_lock(&octx->uring_lock);
+ return 0;
+}
+
void io_msg_ring_cleanup(struct io_kiocb *req)
{
struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
@@ -51,29 +73,6 @@ static int io_msg_ring_data(struct io_kiocb *req)
return -EOVERFLOW;
}
-static void io_double_unlock_ctx(struct io_ring_ctx *octx,
- unsigned int issue_flags)
-{
- mutex_unlock(&octx->uring_lock);
-}
-
-static int io_double_lock_ctx(struct io_ring_ctx *octx,
- unsigned int issue_flags)
-{
- /*
- * To ensure proper ordering between the two ctxs, we can only
- * attempt a trylock on the target. If that fails and we already have
- * the source ctx lock, punt to io-wq.
- */
- if (!(issue_flags & IO_URING_F_UNLOCKED)) {
- if (!mutex_trylock(&octx->uring_lock))
- return -EAGAIN;
- return 0;
- }
- mutex_lock(&octx->uring_lock);
- return 0;
-}
-
static struct file *io_msg_grab_file(struct io_kiocb *req, unsigned int issue_flags)
{
struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
@@ -122,7 +121,7 @@ static int io_msg_install_complete(struct io_kiocb *req, unsigned int issue_flag
if (!io_post_aux_cqe(target_ctx, msg->user_data, ret, 0, true))
ret = -EOVERFLOW;
out_unlock:
- io_double_unlock_ctx(target_ctx, issue_flags);
+ io_double_unlock_ctx(target_ctx);
return ret;
}
--
2.40.1
[-- Attachment #4: 0002-io_uring-extract-a-io_msg_install_complete-helper.patch --]
[-- Type: text/x-patch, Size: 2957 bytes --]
From ef7b782465bd6943fbcacb4af4fbe790137a5820 Mon Sep 17 00:00:00 2001
From: Pavel Begunkov <[email protected]>
Date: Wed, 7 Dec 2022 03:53:35 +0000
Subject: [PATCH 2/4] io_uring: extract a io_msg_install_complete helper
Commit 172113101641cf1f9628c528ec790cb809f2b704 upstream.
Extract a helper called io_msg_install_complete() from io_msg_send_fd(),
will be used later.
Signed-off-by: Pavel Begunkov <[email protected]>
Link: https://lore.kernel.org/r/1500ca1054cc4286a3ee1c60aacead57fcdfa02a.1670384893.git.asml.silence@gmail.com
Signed-off-by: Jens Axboe <[email protected]>
---
io_uring/msg_ring.c | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index ee8e7ac8c582..fd96a0cd85f0 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -94,40 +94,25 @@ static struct file *io_msg_grab_file(struct io_kiocb *req, unsigned int issue_fl
return file;
}
-static int io_msg_send_fd(struct io_kiocb *req, unsigned int issue_flags)
+static int io_msg_install_complete(struct io_kiocb *req, unsigned int issue_flags)
{
struct io_ring_ctx *target_ctx = req->file->private_data;
struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
- struct io_ring_ctx *ctx = req->ctx;
struct file *src_file = msg->src_file;
int ret;
- if (msg->len)
- return -EINVAL;
- if (target_ctx == ctx)
- return -EINVAL;
- if (target_ctx->flags & IORING_SETUP_R_DISABLED)
- return -EBADFD;
- if (!src_file) {
- src_file = io_msg_grab_file(req, issue_flags);
- if (!src_file)
- return -EBADF;
- msg->src_file = src_file;
- req->flags |= REQ_F_NEED_CLEANUP;
- }
-
if (unlikely(io_double_lock_ctx(target_ctx, issue_flags)))
return -EAGAIN;
ret = __io_fixed_fd_install(target_ctx, src_file, msg->dst_fd);
if (ret < 0)
goto out_unlock;
+
msg->src_file = NULL;
req->flags &= ~REQ_F_NEED_CLEANUP;
if (msg->flags & IORING_MSG_RING_CQE_SKIP)
goto out_unlock;
-
/*
* If this fails, the target still received the file descriptor but
* wasn't notified of the fact. This means that if this request
@@ -141,6 +126,25 @@ static int io_msg_send_fd(struct io_kiocb *req, unsigned int issue_flags)
return ret;
}
+static int io_msg_send_fd(struct io_kiocb *req, unsigned int issue_flags)
+{
+ struct io_ring_ctx *target_ctx = req->file->private_data;
+ struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
+ struct io_ring_ctx *ctx = req->ctx;
+ struct file *src_file = msg->src_file;
+
+ if (target_ctx == ctx)
+ return -EINVAL;
+ if (!src_file) {
+ src_file = io_msg_grab_file(req, issue_flags);
+ if (!src_file)
+ return -EBADF;
+ msg->src_file = src_file;
+ req->flags |= REQ_F_NEED_CLEANUP;
+ }
+ return io_msg_install_complete(req, issue_flags);
+}
+
int io_msg_ring_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
{
struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
--
2.40.1
[-- Attachment #5: 0001-io_uring-get-rid-of-double-locking.patch --]
[-- Type: text/x-patch, Size: 6081 bytes --]
From f10ba483becae0b8c7de0e9fe3e0f6d07a405b7a Mon Sep 17 00:00:00 2001
From: Pavel Begunkov <[email protected]>
Date: Wed, 7 Dec 2022 03:53:34 +0000
Subject: [PATCH 1/4] io_uring: get rid of double locking
Commit 11373026f2960390d5e330df4e92735c4265c440 upstream.
We don't need to take both uring_locks at once, msg_ring can be split in
two parts, first getting a file from the filetable of the first ring and
then installing it into the second one.
Signed-off-by: Pavel Begunkov <[email protected]>
Link: https://lore.kernel.org/r/a80ecc2bc99c3b3f2cf20015d618b7c51419a797.1670384893.git.asml.silence@gmail.com
Signed-off-by: Jens Axboe <[email protected]>
---
io_uring/msg_ring.c | 85 ++++++++++++++++++++++++++-------------------
io_uring/msg_ring.h | 1 +
io_uring/opdef.c | 1 +
3 files changed, 51 insertions(+), 36 deletions(-)
diff --git a/io_uring/msg_ring.c b/io_uring/msg_ring.c
index 3526389ac218..ee8e7ac8c582 100644
--- a/io_uring/msg_ring.c
+++ b/io_uring/msg_ring.c
@@ -15,6 +15,7 @@
struct io_msg {
struct file *file;
+ struct file *src_file;
u64 user_data;
u32 len;
u32 cmd;
@@ -23,6 +24,17 @@ struct io_msg {
u32 flags;
};
+void io_msg_ring_cleanup(struct io_kiocb *req)
+{
+ struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
+
+ if (WARN_ON_ONCE(!msg->src_file))
+ return;
+
+ fput(msg->src_file);
+ msg->src_file = NULL;
+}
+
static int io_msg_ring_data(struct io_kiocb *req)
{
struct io_ring_ctx *target_ctx = req->file->private_data;
@@ -39,17 +51,13 @@ static int io_msg_ring_data(struct io_kiocb *req)
return -EOVERFLOW;
}
-static void io_double_unlock_ctx(struct io_ring_ctx *ctx,
- struct io_ring_ctx *octx,
+static void io_double_unlock_ctx(struct io_ring_ctx *octx,
unsigned int issue_flags)
{
- if (issue_flags & IO_URING_F_UNLOCKED)
- mutex_unlock(&ctx->uring_lock);
mutex_unlock(&octx->uring_lock);
}
-static int io_double_lock_ctx(struct io_ring_ctx *ctx,
- struct io_ring_ctx *octx,
+static int io_double_lock_ctx(struct io_ring_ctx *octx,
unsigned int issue_flags)
{
/*
@@ -62,17 +70,28 @@ static int io_double_lock_ctx(struct io_ring_ctx *ctx,
return -EAGAIN;
return 0;
}
+ mutex_lock(&octx->uring_lock);
+ return 0;
+}
- /* Always grab smallest value ctx first. We know ctx != octx. */
- if (ctx < octx) {
- mutex_lock(&ctx->uring_lock);
- mutex_lock(&octx->uring_lock);
- } else {
- mutex_lock(&octx->uring_lock);
- mutex_lock(&ctx->uring_lock);
+static struct file *io_msg_grab_file(struct io_kiocb *req, unsigned int issue_flags)
+{
+ struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
+ struct io_ring_ctx *ctx = req->ctx;
+ struct file *file = NULL;
+ unsigned long file_ptr;
+ int idx = msg->src_fd;
+
+ io_ring_submit_lock(ctx, issue_flags);
+ if (likely(idx < ctx->nr_user_files)) {
+ idx = array_index_nospec(idx, ctx->nr_user_files);
+ file_ptr = io_fixed_file_slot(&ctx->file_table, idx)->file_ptr;
+ file = (struct file *) (file_ptr & FFS_MASK);
+ if (file)
+ get_file(file);
}
-
- return 0;
+ io_ring_submit_unlock(ctx, issue_flags);
+ return file;
}
static int io_msg_send_fd(struct io_kiocb *req, unsigned int issue_flags)
@@ -80,8 +99,7 @@ static int io_msg_send_fd(struct io_kiocb *req, unsigned int issue_flags)
struct io_ring_ctx *target_ctx = req->file->private_data;
struct io_msg *msg = io_kiocb_to_cmd(req, struct io_msg);
struct io_ring_ctx *ctx = req->ctx;
- unsigned long file_ptr;
- struct file *src_file;
+ struct file *src_file = msg->src_file;
int ret;
if (msg->len)
@@ -90,28 +108,22 @@ static int io_msg_send_fd(struct io_kiocb *req, unsigned int issue_flags)
return -EINVAL;
if (target_ctx->flags & IORING_SETUP_R_DISABLED)
return -EBADFD;
+ if (!src_file) {
+ src_file = io_msg_grab_file(req, issue_flags);
+ if (!src_file)
+ return -EBADF;
+ msg->src_file = src_file;
+ req->flags |= REQ_F_NEED_CLEANUP;
+ }
- ret = io_double_lock_ctx(ctx, target_ctx, issue_flags);
- if (unlikely(ret))
- return ret;
-
- ret = -EBADF;
- if (unlikely(msg->src_fd >= ctx->nr_user_files))
- goto out_unlock;
-
- msg->src_fd = array_index_nospec(msg->src_fd, ctx->nr_user_files);
- file_ptr = io_fixed_file_slot(&ctx->file_table, msg->src_fd)->file_ptr;
- if (!file_ptr)
- goto out_unlock;
-
- src_file = (struct file *) (file_ptr & FFS_MASK);
- get_file(src_file);
+ if (unlikely(io_double_lock_ctx(target_ctx, issue_flags)))
+ return -EAGAIN;
ret = __io_fixed_fd_install(target_ctx, src_file, msg->dst_fd);
- if (ret < 0) {
- fput(src_file);
+ if (ret < 0)
goto out_unlock;
- }
+ msg->src_file = NULL;
+ req->flags &= ~REQ_F_NEED_CLEANUP;
if (msg->flags & IORING_MSG_RING_CQE_SKIP)
goto out_unlock;
@@ -125,7 +137,7 @@ static int io_msg_send_fd(struct io_kiocb *req, unsigned int issue_flags)
if (!io_post_aux_cqe(target_ctx, msg->user_data, ret, 0, true))
ret = -EOVERFLOW;
out_unlock:
- io_double_unlock_ctx(ctx, target_ctx, issue_flags);
+ io_double_unlock_ctx(target_ctx, issue_flags);
return ret;
}
@@ -136,6 +148,7 @@ int io_msg_ring_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
if (unlikely(sqe->buf_index || sqe->personality))
return -EINVAL;
+ msg->src_file = NULL;
msg->user_data = READ_ONCE(sqe->off);
msg->len = READ_ONCE(sqe->len);
msg->cmd = READ_ONCE(sqe->addr);
diff --git a/io_uring/msg_ring.h b/io_uring/msg_ring.h
index fb9601f202d0..3987ee6c0e5f 100644
--- a/io_uring/msg_ring.h
+++ b/io_uring/msg_ring.h
@@ -2,3 +2,4 @@
int io_msg_ring_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe);
int io_msg_ring(struct io_kiocb *req, unsigned int issue_flags);
+void io_msg_ring_cleanup(struct io_kiocb *req);
diff --git a/io_uring/opdef.c b/io_uring/opdef.c
index 04dd2c983fce..3aa0d65c50e3 100644
--- a/io_uring/opdef.c
+++ b/io_uring/opdef.c
@@ -445,6 +445,7 @@ const struct io_op_def io_op_defs[] = {
.name = "MSG_RING",
.prep = io_msg_ring_prep,
.issue = io_msg_ring,
+ .cleanup = io_msg_ring_cleanup,
},
[IORING_OP_FSETXATTR] = {
.needs_file = 1,
--
2.40.1
^ permalink raw reply related [flat|nested] 7+ messages in thread