public inbox for [email protected]
 help / color / mirror / Atom feed
* [PATCH for-next 0/3] timeout fixes & improvements
@ 2022-04-20 12:40 Pavel Begunkov
  2022-04-20 12:40 ` [PATCH 1/3] io_uring: fix nested timeout locking on disarming Pavel Begunkov
                   ` (3 more replies)
  0 siblings, 4 replies; 5+ messages in thread
From: Pavel Begunkov @ 2022-04-20 12:40 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, asml.silence

Fix the recent disarming syz report about deadlocking and brush a bit
up timeout/completion locking.

Pavel Begunkov (3):
  io_uring: fix nested timeout locking on disarming
  io_uring: move tout locking in io_timeout_cancel()
  io_uring: refactor io_disarm_next() locking

 fs/io_uring.c | 46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

-- 
2.36.0


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/3] io_uring: fix nested timeout locking on disarming
  2022-04-20 12:40 [PATCH for-next 0/3] timeout fixes & improvements Pavel Begunkov
@ 2022-04-20 12:40 ` Pavel Begunkov
  2022-04-20 12:40 ` [PATCH 2/3] io_uring: move tout locking in io_timeout_cancel() Pavel Begunkov
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 5+ messages in thread
From: Pavel Begunkov @ 2022-04-20 12:40 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, asml.silence, syzbot+57e67273f92d7f5f1931

WARNING: possible recursive locking detected

syz-executor162/3588 is trying to acquire lock:
ffff888011a453d8 (&ctx->timeout_lock){....}-{2:2}, at: spin_lock_irq include/linux/spinlock.h:379 [inline]
ffff888011a453d8 (&ctx->timeout_lock){....}-{2:2}, at: io_disarm_next+0x545/0xaa0 fs/io_uring.c:2452
but task is already holding lock:
ffff888011a453d8 (&ctx->timeout_lock){....}-{2:2}, at: spin_lock_irq include/linux/spinlock.h:379 [inline]
ffff888011a453d8 (&ctx->timeout_lock){....}-{2:2}, at: io_kill_timeouts+0x4c/0x227 fs/io_uring.c:10432

Call Trace:
 <TASK>
...
 spin_lock_irq include/linux/spinlock.h:379 [inline]
 io_disarm_next+0x545/0xaa0 fs/io_uring.c:2452
 __io_req_complete_post+0x794/0xd90 fs/io_uring.c:2200
 io_kill_timeout fs/io_uring.c:1815 [inline]
 io_kill_timeout+0x210/0x21d fs/io_uring.c:1803
 io_kill_timeouts+0xe2/0x227 fs/io_uring.c:10435
 io_ring_ctx_wait_and_kill+0x1eb/0x360 fs/io_uring.c:10462
 io_uring_release+0x42/0x46 fs/io_uring.c:10483
 __fput+0x277/0x9d0 fs/file_table.c:317
 task_work_run+0xdd/0x1a0 kernel/task_work.c:164
...

Return tw deferred putting back, it's easier than looking after all
potential nested locking. However, instead of filling an CQE on the spot
as it was before delay it as well by io_req_complete_post() via tw.

Reported-by: [email protected]
Fixes: 78bfbdd1a497 ("io_uring: kill io_put_req_deferred()")
Signed-off-by: Pavel Begunkov <[email protected]>
---
 fs/io_uring.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 3905b3ec87b8..2b9a3af9ff42 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1214,6 +1214,7 @@ static int io_close_fixed(struct io_kiocb *req, unsigned int issue_flags);
 
 static enum hrtimer_restart io_link_timeout_fn(struct hrtimer *timer);
 static void io_eventfd_signal(struct io_ring_ctx *ctx);
+static void io_req_tw_post_queue(struct io_kiocb *req, s32 res, u32 cflags);
 
 static struct kmem_cache *req_cachep;
 
@@ -1782,7 +1783,7 @@ static void io_kill_timeout(struct io_kiocb *req, int status)
 		atomic_set(&req->ctx->cq_timeouts,
 			atomic_read(&req->ctx->cq_timeouts) + 1);
 		list_del_init(&req->timeout.list);
-		__io_req_complete_post(req, status, 0);
+		io_req_tw_post_queue(req, status, 0);
 	}
 }
 
@@ -2367,7 +2368,7 @@ static bool io_kill_linked_timeout(struct io_kiocb *req)
 		link->timeout.head = NULL;
 		if (hrtimer_try_to_cancel(&io->timer) != -1) {
 			list_del(&link->timeout.list);
-			__io_req_complete_post(link, -ECANCELED, 0);
+			io_req_tw_post_queue(link, -ECANCELED, 0);
 			return true;
 		}
 	}
@@ -2413,7 +2414,7 @@ static bool io_disarm_next(struct io_kiocb *req)
 		req->flags &= ~REQ_F_ARM_LTIMEOUT;
 		if (link && link->opcode == IORING_OP_LINK_TIMEOUT) {
 			io_remove_next_linked(req);
-			__io_req_complete_post(link, -ECANCELED, 0);
+			io_req_tw_post_queue(link, -ECANCELED, 0);
 			posted = true;
 		}
 	} else if (req->flags & REQ_F_LINK_TIMEOUT) {
@@ -2632,6 +2633,19 @@ static void io_req_task_work_add(struct io_kiocb *req, bool priority)
 	}
 }
 
+static void io_req_tw_post(struct io_kiocb *req, bool *locked)
+{
+	io_req_complete_post(req, req->cqe.res, req->cqe.flags);
+}
+
+static void io_req_tw_post_queue(struct io_kiocb *req, s32 res, u32 cflags)
+{
+	req->cqe.res = res;
+	req->cqe.flags = cflags;
+	req->io_task_work.func = io_req_tw_post;
+	io_req_task_work_add(req, false);
+}
+
 static void io_req_task_cancel(struct io_kiocb *req, bool *locked)
 {
 	/* not needed for normal modes, but SQPOLL depends on it */
-- 
2.36.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/3] io_uring: move tout locking in io_timeout_cancel()
  2022-04-20 12:40 [PATCH for-next 0/3] timeout fixes & improvements Pavel Begunkov
  2022-04-20 12:40 ` [PATCH 1/3] io_uring: fix nested timeout locking on disarming Pavel Begunkov
@ 2022-04-20 12:40 ` Pavel Begunkov
  2022-04-20 12:40 ` [PATCH 3/3] io_uring: refactor io_disarm_next() locking Pavel Begunkov
  2022-04-20 22:24 ` [PATCH for-next 0/3] timeout fixes & improvements Jens Axboe
  3 siblings, 0 replies; 5+ messages in thread
From: Pavel Begunkov @ 2022-04-20 12:40 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, asml.silence

Move ->timeout_lock grabbing inside of io_timeout_cancel(), so
we can do io_req_task_queue_fail() outside of the lock. It's much nicer
than relying on triple nested locking.

Signed-off-by: Pavel Begunkov <[email protected]>
---
 fs/io_uring.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 2b9a3af9ff42..ce5941537b64 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6489,7 +6489,11 @@ static int io_timeout_cancel(struct io_ring_ctx *ctx, __u64 user_data)
 	__must_hold(&ctx->completion_lock)
 	__must_hold(&ctx->timeout_lock)
 {
-	struct io_kiocb *req = io_timeout_extract(ctx, user_data);
+	struct io_kiocb *req;
+
+	spin_lock_irq(&ctx->timeout_lock);
+	req = io_timeout_extract(ctx, user_data);
+	spin_unlock_irq(&ctx->timeout_lock);
 
 	if (IS_ERR(req))
 		return PTR_ERR(req);
@@ -6608,9 +6612,7 @@ static int io_timeout_remove(struct io_kiocb *req, unsigned int issue_flags)
 
 	if (!(req->timeout_rem.flags & IORING_TIMEOUT_UPDATE)) {
 		spin_lock(&ctx->completion_lock);
-		spin_lock_irq(&ctx->timeout_lock);
 		ret = io_timeout_cancel(ctx, tr->addr);
-		spin_unlock_irq(&ctx->timeout_lock);
 		spin_unlock(&ctx->completion_lock);
 	} else {
 		enum hrtimer_mode mode = io_translate_timeout_mode(tr->flags);
@@ -6796,10 +6798,7 @@ static int io_try_cancel_userdata(struct io_kiocb *req, u64 sqe_addr)
 	ret = io_poll_cancel(ctx, sqe_addr, false);
 	if (ret != -ENOENT)
 		goto out;
-
-	spin_lock_irq(&ctx->timeout_lock);
 	ret = io_timeout_cancel(ctx, sqe_addr);
-	spin_unlock_irq(&ctx->timeout_lock);
 out:
 	spin_unlock(&ctx->completion_lock);
 	return ret;
-- 
2.36.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 3/3] io_uring: refactor io_disarm_next() locking
  2022-04-20 12:40 [PATCH for-next 0/3] timeout fixes & improvements Pavel Begunkov
  2022-04-20 12:40 ` [PATCH 1/3] io_uring: fix nested timeout locking on disarming Pavel Begunkov
  2022-04-20 12:40 ` [PATCH 2/3] io_uring: move tout locking in io_timeout_cancel() Pavel Begunkov
@ 2022-04-20 12:40 ` Pavel Begunkov
  2022-04-20 22:24 ` [PATCH for-next 0/3] timeout fixes & improvements Jens Axboe
  3 siblings, 0 replies; 5+ messages in thread
From: Pavel Begunkov @ 2022-04-20 12:40 UTC (permalink / raw)
  To: io-uring; +Cc: Jens Axboe, asml.silence

Split timeout handling into removal + failing, so we can reduce
spinlocking time and remove another instance of triple nested locking.

Signed-off-by: Pavel Begunkov <[email protected]>
---
 fs/io_uring.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ce5941537b64..1572edf97c06 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -2355,7 +2355,7 @@ static inline void io_remove_next_linked(struct io_kiocb *req)
 	nxt->link = NULL;
 }
 
-static bool io_kill_linked_timeout(struct io_kiocb *req)
+static struct io_kiocb *io_disarm_linked_timeout(struct io_kiocb *req)
 	__must_hold(&req->ctx->completion_lock)
 	__must_hold(&req->ctx->timeout_lock)
 {
@@ -2368,11 +2368,10 @@ static bool io_kill_linked_timeout(struct io_kiocb *req)
 		link->timeout.head = NULL;
 		if (hrtimer_try_to_cancel(&io->timer) != -1) {
 			list_del(&link->timeout.list);
-			io_req_tw_post_queue(link, -ECANCELED, 0);
-			return true;
+			return link;
 		}
 	}
-	return false;
+	return NULL;
 }
 
 static void io_fail_links(struct io_kiocb *req)
@@ -2406,11 +2405,11 @@ static void io_fail_links(struct io_kiocb *req)
 static bool io_disarm_next(struct io_kiocb *req)
 	__must_hold(&req->ctx->completion_lock)
 {
+	struct io_kiocb *link = NULL;
 	bool posted = false;
 
 	if (req->flags & REQ_F_ARM_LTIMEOUT) {
-		struct io_kiocb *link = req->link;
-
+		link = req->link;
 		req->flags &= ~REQ_F_ARM_LTIMEOUT;
 		if (link && link->opcode == IORING_OP_LINK_TIMEOUT) {
 			io_remove_next_linked(req);
@@ -2421,8 +2420,12 @@ static bool io_disarm_next(struct io_kiocb *req)
 		struct io_ring_ctx *ctx = req->ctx;
 
 		spin_lock_irq(&ctx->timeout_lock);
-		posted = io_kill_linked_timeout(req);
+		link = io_disarm_linked_timeout(req);
 		spin_unlock_irq(&ctx->timeout_lock);
+		if (link) {
+			posted = true;
+			io_req_tw_post_queue(link, -ECANCELED, 0);
+		}
 	}
 	if (unlikely((req->flags & REQ_F_FAIL) &&
 		     !(req->flags & REQ_F_HARDLINK))) {
-- 
2.36.0


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH for-next 0/3] timeout fixes & improvements
  2022-04-20 12:40 [PATCH for-next 0/3] timeout fixes & improvements Pavel Begunkov
                   ` (2 preceding siblings ...)
  2022-04-20 12:40 ` [PATCH 3/3] io_uring: refactor io_disarm_next() locking Pavel Begunkov
@ 2022-04-20 22:24 ` Jens Axboe
  3 siblings, 0 replies; 5+ messages in thread
From: Jens Axboe @ 2022-04-20 22:24 UTC (permalink / raw)
  To: io-uring, asml.silence

On Wed, 20 Apr 2022 13:40:52 +0100, Pavel Begunkov wrote:
> Fix the recent disarming syz report about deadlocking and brush a bit
> up timeout/completion locking.
> 
> Pavel Begunkov (3):
>   io_uring: fix nested timeout locking on disarming
>   io_uring: move tout locking in io_timeout_cancel()
>   io_uring: refactor io_disarm_next() locking
> 
> [...]

Applied, thanks!

[1/3] io_uring: fix nested timeout locking on disarming
      (no commit info)
[2/3] io_uring: move tout locking in io_timeout_cancel()
      commit: 03b1df2dbcaf77c46f6cc64747fd9151e36842ab
[3/3] io_uring: refactor io_disarm_next() locking
      commit: a850c21e744eda43d5d296a315d5a4208d3e2281

Best regards,
-- 
Jens Axboe



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-04-20 22:24 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-04-20 12:40 [PATCH for-next 0/3] timeout fixes & improvements Pavel Begunkov
2022-04-20 12:40 ` [PATCH 1/3] io_uring: fix nested timeout locking on disarming Pavel Begunkov
2022-04-20 12:40 ` [PATCH 2/3] io_uring: move tout locking in io_timeout_cancel() Pavel Begunkov
2022-04-20 12:40 ` [PATCH 3/3] io_uring: refactor io_disarm_next() locking Pavel Begunkov
2022-04-20 22:24 ` [PATCH for-next 0/3] timeout fixes & improvements Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox