[PATCH for-next 0/3] timeout fixes & improvements

[PATCH for-next 0/3] timeout fixes & improvements

[Pavel Begunkov]

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

[PATCH 1/3] io_uring: fix nested timeout locking on disarming

[Pavel Begunkov]

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: syzbot+57e67273f92d7f5f1931@syzkaller.appspotmail.com
Fixes: 78bfbdd1a497 ("io_uring: kill io_put_req_deferred()")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 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

[PATCH 2/3] io_uring: move tout locking in io_timeout_cancel()

[Pavel Begunkov]

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 <asml.silence@gmail.com>
---
 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

[PATCH 3/3] io_uring: refactor io_disarm_next() locking

[Pavel Begunkov]

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 <asml.silence@gmail.com>
---
 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

Re: [PATCH for-next 0/3] timeout fixes & improvements

[Jens Axboe]

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