[PATCH 5.11] io_uring: fix wqe->lock/completion_lock deadlock

[PATCH 5.11] io_uring: fix wqe->lock/completion_lock deadlock

[Pavel Begunkov]

Joseph reports following deadlock:

CPU0:
...
io_kill_linked_timeout  // &ctx->completion_lock
io_commit_cqring
__io_queue_deferred
__io_queue_async_work
io_wq_enqueue
io_wqe_enqueue  // &wqe->lock

CPU1:
...
__io_uring_files_cancel
io_wq_cancel_cb
io_wqe_cancel_pending_work  // &wqe->lock
io_cancel_task_cb  // &ctx->completion_lock

Only __io_queue_deferred() calls queue_async_work() while holding
ctx->completion_lock, enqueue drained requests via io_req_task_queue()
instead.

Cc: stable@vger.kernel.org # 5.9+
Reported-by: Joseph Qi <joseph.qi@linux.alibaba.com>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index bb0270eeb8cb..c218deaf73a9 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1026,6 +1026,7 @@ static int io_setup_async_rw(struct io_kiocb *req, const struct iovec *iovec,
 			     const struct iovec *fast_iov,
 			     struct iov_iter *iter, bool force);
 static void io_req_drop_files(struct io_kiocb *req);
+static void io_req_task_queue(struct io_kiocb *req);
 
 static struct kmem_cache *req_cachep;
 
@@ -1634,18 +1635,11 @@ static void __io_queue_deferred(struct io_ring_ctx *ctx)
 	do {
 		struct io_defer_entry *de = list_first_entry(&ctx->defer_list,
 						struct io_defer_entry, list);
-		struct io_kiocb *link;
 
 		if (req_need_defer(de->req, de->seq))
 			break;
 		list_del_init(&de->list);
-		/* punt-init is done before queueing for defer */
-		link = __io_queue_async_work(de->req);
-		if (link) {
-			__io_queue_linked_timeout(link);
-			/* drop submission reference */
-			io_put_req_deferred(link, 1);
-		}
+		io_req_task_queue(de->req);
 		kfree(de);
 	} while (!list_empty(&ctx->defer_list));
 }
-- 
2.24.0

Re: [PATCH 5.11] io_uring: fix wqe->lock/completion_lock deadlock

[Pavel Begunkov]

On 26/01/2021 23:35, Pavel Begunkov wrote:
> Joseph reports following deadlock:
> 
> CPU0:
> ...
> io_kill_linked_timeout  // &ctx->completion_lock
> io_commit_cqring
> __io_queue_deferred
> __io_queue_async_work
> io_wq_enqueue
> io_wqe_enqueue  // &wqe->lock
> 
> CPU1:
> ...
> __io_uring_files_cancel
> io_wq_cancel_cb
> io_wqe_cancel_pending_work  // &wqe->lock
> io_cancel_task_cb  // &ctx->completion_lock
> 
> Only __io_queue_deferred() calls queue_async_work() while holding
> ctx->completion_lock, enqueue drained requests via io_req_task_queue()
> instead.

Joseph, can you try it out? would much appreciate

> 
> Cc: stable@vger.kernel.org # 5.9+
> Reported-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index bb0270eeb8cb..c218deaf73a9 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1026,6 +1026,7 @@ static int io_setup_async_rw(struct io_kiocb *req, const struct iovec *iovec,
>  			     const struct iovec *fast_iov,
>  			     struct iov_iter *iter, bool force);
>  static void io_req_drop_files(struct io_kiocb *req);
> +static void io_req_task_queue(struct io_kiocb *req);
>  
>  static struct kmem_cache *req_cachep;
>  
> @@ -1634,18 +1635,11 @@ static void __io_queue_deferred(struct io_ring_ctx *ctx)
>  	do {
>  		struct io_defer_entry *de = list_first_entry(&ctx->defer_list,
>  						struct io_defer_entry, list);
> -		struct io_kiocb *link;
>  
>  		if (req_need_defer(de->req, de->seq))
>  			break;
>  		list_del_init(&de->list);
> -		/* punt-init is done before queueing for defer */
> -		link = __io_queue_async_work(de->req);
> -		if (link) {
> -			__io_queue_linked_timeout(link);
> -			/* drop submission reference */
> -			io_put_req_deferred(link, 1);
> -		}
> +		io_req_task_queue(de->req);
>  		kfree(de);
>  	} while (!list_empty(&ctx->defer_list));
>  }
> 

-- 
Pavel Begunkov

Re: [PATCH 5.11] io_uring: fix wqe->lock/completion_lock deadlock

[Joseph Qi]

On 1/27/21 7:35 AM, Pavel Begunkov wrote:
> Joseph reports following deadlock:
> 
> CPU0:
> ...
> io_kill_linked_timeout  // &ctx->completion_lock
> io_commit_cqring
> __io_queue_deferred
> __io_queue_async_work
> io_wq_enqueue
> io_wqe_enqueue  // &wqe->lock
> 
> CPU1:
> ...
> __io_uring_files_cancel
> io_wq_cancel_cb
> io_wqe_cancel_pending_work  // &wqe->lock
> io_cancel_task_cb  // &ctx->completion_lock
> 
> Only __io_queue_deferred() calls queue_async_work() while holding
> ctx->completion_lock, enqueue drained requests via io_req_task_queue()
> instead.
> 
We should follow &wqe->lock > &ctx->completion_lock from now on, right?
I was thinking getting completion_lock first before:(

Moreover, there are so many locks and no suggested locking order in
comments, so that it is hard for us to participate in the work.

> Cc: stable@vger.kernel.org # 5.9+
> Reported-by: Joseph Qi <joseph.qi@linux.alibaba.com>

Tested-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index bb0270eeb8cb..c218deaf73a9 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1026,6 +1026,7 @@ static int io_setup_async_rw(struct io_kiocb *req, const struct iovec *iovec,
>  			     const struct iovec *fast_iov,
>  			     struct iov_iter *iter, bool force);
>  static void io_req_drop_files(struct io_kiocb *req);
> +static void io_req_task_queue(struct io_kiocb *req);
>  
>  static struct kmem_cache *req_cachep;
>  
> @@ -1634,18 +1635,11 @@ static void __io_queue_deferred(struct io_ring_ctx *ctx)
>  	do {
>  		struct io_defer_entry *de = list_first_entry(&ctx->defer_list,
>  						struct io_defer_entry, list);
> -		struct io_kiocb *link;
>  
>  		if (req_need_defer(de->req, de->seq))
>  			break;
>  		list_del_init(&de->list);
> -		/* punt-init is done before queueing for defer */
> -		link = __io_queue_async_work(de->req);
> -		if (link) {
> -			__io_queue_linked_timeout(link);
> -			/* drop submission reference */
> -			io_put_req_deferred(link, 1);
> -		}
> +		io_req_task_queue(de->req);
>  		kfree(de);
>  	} while (!list_empty(&ctx->defer_list));
>  }
> 

Re: [PATCH 5.11] io_uring: fix wqe->lock/completion_lock deadlock

[Jens Axboe]

On 1/26/21 6:52 PM, Joseph Qi wrote:
> 
> 
> On 1/27/21 7:35 AM, Pavel Begunkov wrote:
>> Joseph reports following deadlock:
>>
>> CPU0:
>> ...
>> io_kill_linked_timeout  // &ctx->completion_lock
>> io_commit_cqring
>> __io_queue_deferred
>> __io_queue_async_work
>> io_wq_enqueue
>> io_wqe_enqueue  // &wqe->lock
>>
>> CPU1:
>> ...
>> __io_uring_files_cancel
>> io_wq_cancel_cb
>> io_wqe_cancel_pending_work  // &wqe->lock
>> io_cancel_task_cb  // &ctx->completion_lock
>>
>> Only __io_queue_deferred() calls queue_async_work() while holding
>> ctx->completion_lock, enqueue drained requests via io_req_task_queue()
>> instead.
>>
> We should follow &wqe->lock > &ctx->completion_lock from now on, right?
> I was thinking getting completion_lock first before:(
> 
> Moreover, there are so many locks and no suggested locking order in
> comments, so that it is hard for us to participate in the work.

So many locks? There's really not even a handful, and only a few that
have overlaps (and hence ordering). Other sub-systems are have a crap
ton more :-). But I can document the ordering, at least that's a start
even if things like that tend to go stale. 

Thanks for reporting and testing! I'll queue this up for 5.11.

-- 
Jens Axboe

Re: [PATCH 5.11] io_uring: fix wqe->lock/completion_lock deadlock

[Pavel Begunkov]

On 27/01/2021 01:52, Joseph Qi wrote:>> Only __io_queue_deferred() calls queue_async_work() while holding
>> ctx->completion_lock, enqueue drained requests via io_req_task_queue()
>> instead.
>>
> We should follow &wqe->lock > &ctx->completion_lock from now on, right?
> I was thinking getting completion_lock first before:(
> 
> Moreover, there are so many locks and no suggested locking order in
> comments, so that it is hard for us to participate in the work.

It's rc5, so that was rather of a "make it fast" kind... I don't like
this ordering, but hopefully the patch doesn't enforce it, and I'd leave
it for 5.11 and prefer to see something else for next releases.

Would be glad to review if take on it. I think it's no good taking a
lock before io_wq_cancel_cb(), but there should be other options. E.g.
split wqe->lock in two.

> 
>> Cc: stable@vger.kernel.org # 5.9+
>> Reported-by: Joseph Qi <joseph.qi@linux.alibaba.com>
> 
> Tested-by: Joseph Qi <joseph.qi@linux.alibaba.com>

Thanks!

-- 
Pavel Begunkov