We should make sure that async workqueue is canceled on exit, but on
some corner case, we found that the async workqueue is not canceled
on exit in the linux-5.4. So we started an in-depth investigation.
Fortunately, we finally found the problem. The commit:
1c4404efcf2c ("io_uring: make sure async workqueue is canceled on exit")
did not completely solve this problem. This patch series to solve this
problem completely. And there's no upstream variant of this commit, so
this patch series is just fix the linux-5.4.y stable branch.
changelog in v3:
1. Merge patch-4 to patch-1.
changelog in v2:
1. Fix missing save the current thread files
2. Fix double list add in io_queue_async_work()
Muchun Song (4):
io_uring: Fix missing smp_mb() in io_cancel_async_work()
io_uring: Fix remove irrelevant req from the task_list
io_uring: Fix missing save the current thread files
io_uring: Fix double list add in io_queue_async_work()
Yinyin Zhu (1):
io_uring: Fix resource leaking when kill the process
fs/io_uring.c | 59 +++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 39 insertions(+), 20 deletions(-)
--
2.11.0
If we queue work in io_poll_wake(), it will leads to list double
add. So we should add the list when the callback func is the
io_sq_wq_submit_work.
The following oops was seen:
list_add double add: new=ffff9ca6a8f1b0e0, prev=ffff9ca62001cee8,
next=ffff9ca6a8f1b0e0.
------------[ cut here ]------------
kernel BUG at lib/list_debug.c:31!
Call Trace:
<IRQ>
io_poll_wake+0xf3/0x230
__wake_up_common+0x91/0x170
__wake_up_common_lock+0x7a/0xc0
io_commit_cqring+0xea/0x280
? blkcg_iolatency_done_bio+0x2b/0x610
io_cqring_add_event+0x3e/0x60
io_complete_rw+0x58/0x80
dio_complete+0x106/0x250
blk_update_request+0xa0/0x3b0
blk_mq_end_request+0x1a/0x110
blk_mq_complete_request+0xd0/0xe0
nvme_irq+0x129/0x270 [nvme]
__handle_irq_event_percpu+0x7b/0x190
handle_irq_event_percpu+0x30/0x80
handle_irq_event+0x3c/0x60
handle_edge_irq+0x91/0x1e0
do_IRQ+0x4d/0xd0
common_interrupt+0xf/0xf
Fixes: 1c4404efcf2c ("io_uring: make sure async workqueue is canceled on exit")
Reported-by: Jiachen Zhang <[email protected]>
Signed-off-by: Muchun Song <[email protected]>
---
fs/io_uring.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index c65f78f395655..a7cfe976480d8 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -513,12 +513,14 @@ static inline void io_queue_async_work(struct io_ring_ctx *ctx,
}
}
- req->files = current->files;
+ if (req->work.func == io_sq_wq_submit_work) {
+ req->files = current->files;
- spin_lock_irqsave(&ctx->task_lock, flags);
- list_add(&req->task_list, &ctx->task_list);
- req->work_task = NULL;
- spin_unlock_irqrestore(&ctx->task_lock, flags);
+ spin_lock_irqsave(&ctx->task_lock, flags);
+ list_add(&req->task_list, &ctx->task_list);
+ req->work_task = NULL;
+ spin_unlock_irqrestore(&ctx->task_lock, flags);
+ }
queue_work(ctx->sqo_wq[rw], &req->work);
}
@@ -667,6 +669,7 @@ static struct io_kiocb *io_get_req(struct io_ring_ctx *ctx,
state->cur_req++;
}
+ INIT_LIST_HEAD(&req->task_list);
req->file = NULL;
req->ctx = ctx;
req->flags = 0;
--
2.11.0
If the process 0 has been initialized io_uring is complete, and
then fork process 1. If process 1 exits and it leads to delete
all reqs from the task_list. If we kill process 0. We will not
send SIGINT signal to the kworker. So we can not remove the req
from the task_list. The io_sq_wq_submit_work() can do that for
us.
Fixes: 1c4404efcf2c ("io_uring: make sure async workqueue is canceled on exit")
Signed-off-by: Muchun Song <[email protected]>
---
fs/io_uring.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 5d9583e3d0d25..c65f78f395655 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2277,13 +2277,11 @@ static void io_sq_wq_submit_work(struct work_struct *work)
break;
cond_resched();
} while (1);
-end_req:
- if (!list_empty(&req->task_list)) {
- spin_lock_irq(&ctx->task_lock);
- list_del_init(&req->task_list);
- spin_unlock_irq(&ctx->task_lock);
- }
}
+end_req:
+ spin_lock_irq(&ctx->task_lock);
+ list_del_init(&req->task_list);
+ spin_unlock_irq(&ctx->task_lock);
/* drop submission reference */
io_put_req(req);
@@ -3727,15 +3725,16 @@ static int io_uring_fasync(int fd, struct file *file, int on)
static void io_cancel_async_work(struct io_ring_ctx *ctx,
struct files_struct *files)
{
+ struct io_kiocb *req;
+
if (list_empty(&ctx->task_list))
return;
spin_lock_irq(&ctx->task_lock);
- while (!list_empty(&ctx->task_list)) {
- struct io_kiocb *req;
- req = list_first_entry(&ctx->task_list, struct io_kiocb, task_list);
- list_del_init(&req->task_list);
+ list_for_each_entry(req, &ctx->task_list, task_list) {
+ if (files && req->files != files)
+ continue;
/*
* The below executes an smp_mb(), which matches with the
@@ -3745,7 +3744,7 @@ static void io_cancel_async_work(struct io_ring_ctx *ctx,
*/
smp_store_mb(req->flags, req->flags | REQ_F_CANCEL); /* B */
- if (req->work_task && (!files || req->files == files))
+ if (req->work_task)
send_sig(SIGINT, req->work_task, 1);
}
spin_unlock_irq(&ctx->task_lock);
--
2.11.0
The store to req->flags and load req->work_task should not be
reordering in io_cancel_async_work(). We should make sure that
either we store REQ_F_CANCE flag to req->flags or we see the
req->work_task setted in io_sq_wq_submit_work().
Fixes: 1c4404efcf2c ("io_uring: make sure async workqueue is canceled on exit")
Signed-off-by: Muchun Song <[email protected]>
---
fs/io_uring.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c
index 2f46def7f5832..5d9583e3d0d25 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2252,6 +2252,12 @@ static void io_sq_wq_submit_work(struct work_struct *work)
if (!ret) {
req->work_task = current;
+
+ /*
+ * Pairs with the smp_store_mb() (B) in
+ * io_cancel_async_work().
+ */
+ smp_mb(); /* A */
if (req->flags & REQ_F_CANCEL) {
ret = -ECANCELED;
goto end_req;
@@ -3730,7 +3736,15 @@ static void io_cancel_async_work(struct io_ring_ctx *ctx,
req = list_first_entry(&ctx->task_list, struct io_kiocb, task_list);
list_del_init(&req->task_list);
- req->flags |= REQ_F_CANCEL;
+
+ /*
+ * The below executes an smp_mb(), which matches with the
+ * smp_mb() (A) in io_sq_wq_submit_work() such that either
+ * we store REQ_F_CANCEL flag to req->flags or we see the
+ * req->work_task setted in io_sq_wq_submit_work().
+ */
+ smp_store_mb(req->flags, req->flags | REQ_F_CANCEL); /* B */
+
if (req->work_task && (!files || req->files == files))
send_sig(SIGINT, req->work_task, 1);
}
--
2.11.0
On 10/6/20 9:16 PM, Muchun Song wrote:
> We should make sure that async workqueue is canceled on exit, but on
> some corner case, we found that the async workqueue is not canceled
> on exit in the linux-5.4. So we started an in-depth investigation.
> Fortunately, we finally found the problem. The commit:
>
> 1c4404efcf2c ("io_uring: make sure async workqueue is canceled on exit")
>
> did not completely solve this problem. This patch series to solve this
> problem completely. And there's no upstream variant of this commit, so
> this patch series is just fix the linux-5.4.y stable branch.
Thanks, I'll review/test these and pass them on if good.
--
Jens Axboe