[PATCH 0/8] iopoll and task_work fixes

[PATCH 0/8] iopoll and task_work fixes

[Pavel Begunkov]

[1-3] are iopoll fixes, where a bug in [1] I unfortenatuly added
yesterday. [4-6] are task_work related.

Tell me, if something from it is needed for 5.8

Pavel Begunkov (8):
  io_uring: fix io_fail_links() locking
  io_uring: fix commit_cqring() locking in iopoll
  io_uring: fix ignoring eventfd in iopoll
  io_uring: fix missing ->mm on exit
  io_uring: don't fail iopoll requeue without ->mm
  io_uring: fix NULL mm in io_poll_task_func()
  io_uring: simplify io_async_task_func()
  io_uring: optimise io_req_find_next() fast check

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

-- 
2.24.0

[PATCH 1/8] io_uring: fix io_fail_links() locking

[Pavel Begunkov]

86b71d0daee05 ("io_uring: deduplicate freeing linked timeouts")
actually fixed one bug, where io_fail_links() doesn't consider
REQ_F_COMP_LOCKED, but added another -- io_cqring_fill_event()
without any locking

Return locking back there and do it right with REQ_F_COMP_LOCKED
check.

Fixes: 86b71d0daee05 ("io_uring: deduplicate freeing linked timeouts")
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 5928404acb17..a1ea41b7b811 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1620,6 +1620,10 @@ static struct io_kiocb *io_req_link_next(struct io_kiocb *req)
 static void io_fail_links(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
+	unsigned long flags = 0;
+
+	if (!(req->flags & REQ_F_COMP_LOCKED))
+		spin_lock_irqsave(&ctx->completion_lock, flags);
 
 	while (!list_empty(&req->link_list)) {
 		struct io_kiocb *link = list_first_entry(&req->link_list,
@@ -1634,6 +1638,8 @@ static void io_fail_links(struct io_kiocb *req)
 	}
 
 	io_commit_cqring(ctx);
+	if (!(req->flags & REQ_F_COMP_LOCKED))
+		spin_unlock_irqrestore(&ctx->completion_lock, flags);
 	io_cqring_ev_posted(ctx);
 }
 
-- 
2.24.0

[PATCH 2/8] io_uring: fix commit_cqring() locking in iopoll

[Pavel Begunkov]

Don't call io_commit_cqring() without holding the completion spinlock
in io_iopoll_complete(), it can race, e.g. with async request failing.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index a1ea41b7b811..96fcdd189ac0 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1923,7 +1923,10 @@ static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
 			io_req_free_batch(&rb, req);
 	}
 
+	spin_lock_irq(&ctx->completion_lock);
 	io_commit_cqring(ctx);
+	spin_unlock_irq(&ctx->completion_lock);
+
 	if (ctx->flags & IORING_SETUP_SQPOLL)
 		io_cqring_ev_posted(ctx);
 	io_req_free_batch_finish(ctx, &rb);
-- 
2.24.0

[PATCH 3/8] io_uring: fix ignoring eventfd in iopoll

[Pavel Begunkov]

io_cqring_ev_posted() also signals eventfd, and nothing prohibits
having eventfd with IOPOLL. The problem is that it will be ignored
in io_iopoll_complete() in non IORING_SETUP_SQPOLL case.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 96fcdd189ac0..776f593a5bf3 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1926,9 +1926,7 @@ static void io_iopoll_complete(struct io_ring_ctx *ctx, unsigned int *nr_events,
 	spin_lock_irq(&ctx->completion_lock);
 	io_commit_cqring(ctx);
 	spin_unlock_irq(&ctx->completion_lock);
-
-	if (ctx->flags & IORING_SETUP_SQPOLL)
-		io_cqring_ev_posted(ctx);
+	io_cqring_ev_posted(ctx);
 	io_req_free_batch_finish(ctx, &rb);
 
 	if (!list_empty(&again))
-- 
2.24.0

[PATCH 4/8] io_uring: fix missing ->mm on exit

[Pavel Begunkov]

There is a fancy bug, where exiting user task may not have ->mm,
that makes task_works to try to do kthread_use_mm(ctx->sqo_mm).

Don't do that if sqo_mm is NULL.

[  290.460558] WARNING: CPU: 6 PID: 150933 at kernel/kthread.c:1238
	kthread_use_mm+0xf3/0x110
[  290.460579] CPU: 6 PID: 150933 Comm: read-write2 Tainted: G
	I E     5.8.0-rc2-00066-g9b21720607cf #531
[  290.460580] RIP: 0010:kthread_use_mm+0xf3/0x110
...
[  290.460584] Call Trace:
[  290.460584]  __io_sq_thread_acquire_mm.isra.0.part.0+0x25/0x30
[  290.460584]  __io_req_task_submit+0x64/0x80
[  290.460584]  io_req_task_submit+0x15/0x20
[  290.460585]  task_work_run+0x67/0xa0
[  290.460585]  do_exit+0x35d/0xb70
[  290.460585]  do_group_exit+0x43/0xa0
[  290.460585]  get_signal+0x140/0x900
[  290.460586]  do_signal+0x37/0x780
[  290.460586]  __prepare_exit_to_usermode+0x126/0x1c0
[  290.460586]  __syscall_return_slowpath+0x3b/0x1c0
[  290.460587]  do_syscall_64+0x5f/0xa0
[  290.460587]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

following with faults.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 776f593a5bf3..c7986c27272e 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -958,7 +958,7 @@ static void io_sq_thread_drop_mm(struct io_ring_ctx *ctx)
 static int __io_sq_thread_acquire_mm(struct io_ring_ctx *ctx)
 {
 	if (!current->mm) {
-		if (unlikely(!mmget_not_zero(ctx->sqo_mm)))
+		if (unlikely(!ctx->sqo_mm || !mmget_not_zero(ctx->sqo_mm)))
 			return -EFAULT;
 		kthread_use_mm(ctx->sqo_mm);
 	}
@@ -7212,10 +7212,10 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx,
 {
 	int ret;
 
-	mmgrab(current->mm);
-	ctx->sqo_mm = current->mm;
-
 	if (ctx->flags & IORING_SETUP_SQPOLL) {
+		mmgrab(current->mm);
+		ctx->sqo_mm = current->mm;
+
 		ret = -EPERM;
 		if (!capable(CAP_SYS_ADMIN))
 			goto err;
@@ -7259,8 +7259,10 @@ static int io_sq_offload_start(struct io_ring_ctx *ctx,
 	return 0;
 err:
 	io_finish_async(ctx);
-	mmdrop(ctx->sqo_mm);
-	ctx->sqo_mm = NULL;
+	if (ctx->sqo_mm) {
+		mmdrop(ctx->sqo_mm);
+		ctx->sqo_mm = NULL;
+	}
 	return ret;
 }
 
-- 
2.24.0

[PATCH 5/8] io_uring: don't fail iopoll requeue without ->mm

[Pavel Begunkov]

Actually, io_iopoll_queue() may have NULL ->mm, that's if SQ thread
didn't grabbed mm before doing iopoll. Don't fail reqs there, as after
recent changes it won't be punted directly but rather through task_work.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index c7986c27272e..589cc157e29c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1881,9 +1881,7 @@ static void io_iopoll_queue(struct list_head *again)
 	do {
 		req = list_first_entry(again, struct io_kiocb, list);
 		list_del(&req->list);
-
-		/* should have ->mm unless io_uring is dying, kill reqs then */
-		if (unlikely(!current->mm) || !io_rw_reissue(req, -EAGAIN))
+		if (!io_rw_reissue(req, -EAGAIN))
 			io_complete_rw_common(&req->rw.kiocb, -EAGAIN, NULL);
 	} while (!list_empty(again));
 }
-- 
2.24.0

[PATCH 6/8] io_uring: fix NULL mm in io_poll_task_func()

[Pavel Begunkov]

io_poll_task_func() hand-coded link submission forgetting to set
TASK_RUNNING, acquire mm, etc. Call existing helper for that,
i.e. __io_req_task_submit().

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 589cc157e29c..57c194de9165 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4492,13 +4492,8 @@ static void io_poll_task_func(struct callback_head *cb)
 	struct io_kiocb *nxt = NULL;
 
 	io_poll_task_handler(req, &nxt);
-	if (nxt) {
-		struct io_ring_ctx *ctx = nxt->ctx;
-
-		mutex_lock(&ctx->uring_lock);
-		__io_queue_sqe(nxt, NULL, NULL);
-		mutex_unlock(&ctx->uring_lock);
-	}
+	if (nxt)
+		__io_req_task_submit(nxt);
 }
 
 static int io_poll_double_wake(struct wait_queue_entry *wait, unsigned mode,
-- 
2.24.0

[PATCH 7/8] io_uring: simplify io_async_task_func()

[Pavel Begunkov]

Greatly simplify io_async_task_func() removing duplicated functionality
of __io_req_task_submit(). This do one extra spin lock/unlock for
cancelled poll case, but that shouldn't happen often.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 29 +++++------------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 57c194de9165..68dcc29c5dc5 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4582,7 +4582,6 @@ static void io_async_task_func(struct callback_head *cb)
 	struct io_kiocb *req = container_of(cb, struct io_kiocb, task_work);
 	struct async_poll *apoll = req->apoll;
 	struct io_ring_ctx *ctx = req->ctx;
-	bool canceled = false;
 
 	trace_io_uring_task_run(req->ctx, req->opcode, req->user_data);
 
@@ -4592,15 +4591,8 @@ static void io_async_task_func(struct callback_head *cb)
 	}
 
 	/* If req is still hashed, it cannot have been canceled. Don't check. */
-	if (hash_hashed(&req->hash_node)) {
+	if (hash_hashed(&req->hash_node))
 		hash_del(&req->hash_node);
-	} else {
-		canceled = READ_ONCE(apoll->poll.canceled);
-		if (canceled) {
-			io_cqring_fill_event(req, -ECANCELED);
-			io_commit_cqring(ctx);
-		}
-	}
 
 	spin_unlock_irq(&ctx->completion_lock);
 
@@ -4609,21 +4601,10 @@ static void io_async_task_func(struct callback_head *cb)
 		memcpy(&req->work, &apoll->work, sizeof(req->work));
 	kfree(apoll);
 
-	if (!canceled) {
-		__set_current_state(TASK_RUNNING);
-		if (io_sq_thread_acquire_mm(ctx, req)) {
-			io_cqring_add_event(req, -EFAULT, 0);
-			goto end_req;
-		}
-		mutex_lock(&ctx->uring_lock);
-		__io_queue_sqe(req, NULL, NULL);
-		mutex_unlock(&ctx->uring_lock);
-	} else {
-		io_cqring_ev_posted(ctx);
-end_req:
-		req_set_fail_links(req);
-		io_double_put_req(req);
-	}
+	if (!READ_ONCE(apoll->poll.canceled))
+		__io_req_task_submit(req);
+	else
+		__io_req_task_cancel(req, -ECANCELED);
 }
 
 static int io_async_wake(struct wait_queue_entry *wait, unsigned mode, int sync,
-- 
2.24.0

[PATCH 8/8] io_uring: optimise io_req_find_next() fast check

[Pavel Begunkov]

gcc 9.2.0 compiles io_req_find_next() as a separate function leaving
the first REQ_F_LINK_HEAD fast check not inlined. Help it by splitting
out the check from the function.

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 68dcc29c5dc5..8cac266b4674 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1643,12 +1643,9 @@ static void io_fail_links(struct io_kiocb *req)
 	io_cqring_ev_posted(ctx);
 }
 
-static struct io_kiocb *io_req_find_next(struct io_kiocb *req)
+static struct io_kiocb *__io_req_find_next(struct io_kiocb *req)
 {
-	if (likely(!(req->flags & REQ_F_LINK_HEAD)))
-		return NULL;
 	req->flags &= ~REQ_F_LINK_HEAD;
-
 	if (req->flags & REQ_F_LINK_TIMEOUT)
 		io_kill_linked_timeout(req);
 
@@ -1664,6 +1661,13 @@ static struct io_kiocb *io_req_find_next(struct io_kiocb *req)
 	return NULL;
 }
 
+static struct io_kiocb *io_req_find_next(struct io_kiocb *req)
+{
+	if (likely(!(req->flags & REQ_F_LINK_HEAD)))
+		return NULL;
+	return __io_req_find_next(req);
+}
+
 static void __io_req_task_cancel(struct io_kiocb *req, int error)
 {
 	struct io_ring_ctx *ctx = req->ctx;
-- 
2.24.0

Re: [PATCH 0/8] iopoll and task_work fixes

[Pavel Begunkov]

On 30/06/2020 15:20, Pavel Begunkov wrote:
> [1-3] are iopoll fixes, where a bug in [1] I unfortenatuly added
> yesterday. [4-6] are task_work related.

I think there are even more bugs. I'll just leave it here, if somebody
wants to take a look.

- I have a hunch that linked timeouts are broken, probably all these
re-submits cause double io_queue_linked_timeout().

- select-buf may be if not leaking, then not reported to a user
in some cases. Worth to check.

- timeout-overflow test failed for me sometime ago. I think that's bulk
completion related. I sent a patch for a very similar bug, but it got
lost.


> Tell me, if something from it is needed for 5.8
> 
> Pavel Begunkov (8):
>   io_uring: fix io_fail_links() locking
>   io_uring: fix commit_cqring() locking in iopoll
>   io_uring: fix ignoring eventfd in iopoll
>   io_uring: fix missing ->mm on exit
>   io_uring: don't fail iopoll requeue without ->mm
>   io_uring: fix NULL mm in io_poll_task_func()
>   io_uring: simplify io_async_task_func()
>   io_uring: optimise io_req_find_next() fast check
> 
>  fs/io_uring.c | 79 +++++++++++++++++++++------------------------------
>  1 file changed, 33 insertions(+), 46 deletions(-)
> 

-- 
Pavel Begunkov

Re: [PATCH 2/8] io_uring: fix commit_cqring() locking in iopoll

[Jens Axboe]

On 6/30/20 6:20 AM, Pavel Begunkov wrote:
> Don't call io_commit_cqring() without holding the completion spinlock
> in io_iopoll_complete(), it can race, e.g. with async request failing.

Can you be more specific?

-- 
Jens Axboe

Re: [PATCH 2/8] io_uring: fix commit_cqring() locking in iopoll

[Pavel Begunkov]

On 30/06/2020 17:04, Jens Axboe wrote:
> On 6/30/20 6:20 AM, Pavel Begunkov wrote:
>> Don't call io_commit_cqring() without holding the completion spinlock
>> in io_iopoll_complete(), it can race, e.g. with async request failing.
> 
> Can you be more specific?

io_iopoll_complete()
	-> io_req_free_batch()
		-> io_queue_next()
			-> io_req_task_queue()
				-> task_work_add()

if this task_work_add() fails, it will be redirected to io-wq manager task
to do io_req_task_cancel() -> commit_cqring().


And probably something similar will happen if a request currently in io-wq
is retried with
io_rw_should_retry() -> io_async_buf_func() -> task_work_add()


I'll resend patch/series later with a better description. 

-- 
Pavel Begunkov

Re: [PATCH 1/8] io_uring: fix io_fail_links() locking

[Jens Axboe]

On 6/30/20 6:20 AM, Pavel Begunkov wrote:
> 86b71d0daee05 ("io_uring: deduplicate freeing linked timeouts")
> actually fixed one bug, where io_fail_links() doesn't consider
> REQ_F_COMP_LOCKED, but added another -- io_cqring_fill_event()
> without any locking
> 
> Return locking back there and do it right with REQ_F_COMP_LOCKED
> check.

Something like the below is much better, and it also makes it so that
the static analyzers don't throw a fit. I'm going to fold this into
the original.


diff --git a/fs/io_uring.c b/fs/io_uring.c
index e1410ff31892..a0aea78162a6 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1600,7 +1600,7 @@ static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr)
 /*
  * Called if REQ_F_LINK_HEAD is set, and we fail the head request
  */
-static void io_fail_links(struct io_kiocb *req)
+static void __io_fail_links(struct io_kiocb *req)
 {
 	struct io_ring_ctx *ctx = req->ctx;
 
@@ -1620,6 +1620,23 @@ static void io_fail_links(struct io_kiocb *req)
 	io_cqring_ev_posted(ctx);
 }
 
+static void io_fail_links(struct io_kiocb *req)
+{
+	struct io_ring_ctx *ctx = req->ctx;
+
+	if (!(req->flags & REQ_F_COMP_LOCKED)) {
+		unsigned long flags;
+
+		spin_lock_irqsave(&ctx->completion_lock, flags);
+		__io_fail_links(req);
+		spin_unlock_irqrestore(&ctx->completion_lock, flags);
+	} else {
+		__io_fail_links(req);
+	}
+
+	io_cqring_ev_posted(ctx);
+}
+
 static void io_req_find_next(struct io_kiocb *req, struct io_kiocb **nxt)
 {
 	if (likely(!(req->flags & REQ_F_LINK_HEAD)))

-- 
Jens Axboe

Re: [PATCH 1/8] io_uring: fix io_fail_links() locking

[Pavel Begunkov]

On 30/06/2020 17:38, Jens Axboe wrote:
> On 6/30/20 6:20 AM, Pavel Begunkov wrote:
>> 86b71d0daee05 ("io_uring: deduplicate freeing linked timeouts")
>> actually fixed one bug, where io_fail_links() doesn't consider
>> REQ_F_COMP_LOCKED, but added another -- io_cqring_fill_event()
>> without any locking
>>
>> Return locking back there and do it right with REQ_F_COMP_LOCKED
>> check.
> 
> Something like the below is much better, and it also makes it so that
> the static analyzers don't throw a fit. I'm going to fold this into
> the original.

Good point about analyzers, even uninitialised flags there is a warning.
This pattern usually prevents inlining, but don't care about
io_fail_links().

> 
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index e1410ff31892..a0aea78162a6 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -1600,7 +1600,7 @@ static void io_req_link_next(struct io_kiocb *req, struct io_kiocb **nxtptr)
>  /*
>   * Called if REQ_F_LINK_HEAD is set, and we fail the head request
>   */
> -static void io_fail_links(struct io_kiocb *req)
> +static void __io_fail_links(struct io_kiocb *req)
>  {
>  	struct io_ring_ctx *ctx = req->ctx;
>  
> @@ -1620,6 +1620,23 @@ static void io_fail_links(struct io_kiocb *req)
>  	io_cqring_ev_posted(ctx);
>  }
>  
> +static void io_fail_links(struct io_kiocb *req)
> +{
> +	struct io_ring_ctx *ctx = req->ctx;
> +
> +	if (!(req->flags & REQ_F_COMP_LOCKED)) {
> +		unsigned long flags;
> +
> +		spin_lock_irqsave(&ctx->completion_lock, flags);
> +		__io_fail_links(req);
> +		spin_unlock_irqrestore(&ctx->completion_lock, flags);
> +	} else {
> +		__io_fail_links(req);
> +	}
> +
> +	io_cqring_ev_posted(ctx);
> +}
> +
>  static void io_req_find_next(struct io_kiocb *req, struct io_kiocb **nxt)
>  {
>  	if (likely(!(req->flags & REQ_F_LINK_HEAD)))
> 

-- 
Pavel Begunkov

Re: [PATCH 2/8] io_uring: fix commit_cqring() locking in iopoll

[Jens Axboe]

On 6/30/20 8:36 AM, Pavel Begunkov wrote:
> On 30/06/2020 17:04, Jens Axboe wrote:
>> On 6/30/20 6:20 AM, Pavel Begunkov wrote:
>>> Don't call io_commit_cqring() without holding the completion spinlock
>>> in io_iopoll_complete(), it can race, e.g. with async request failing.
>>
>> Can you be more specific?
> 
> io_iopoll_complete()
> 	-> io_req_free_batch()
> 		-> io_queue_next()
> 			-> io_req_task_queue()
> 				-> task_work_add()
> 
> if this task_work_add() fails, it will be redirected to io-wq manager task
> to do io_req_task_cancel() -> commit_cqring().
> 
> 
> And probably something similar will happen if a request currently in io-wq
> is retried with
> io_rw_should_retry() -> io_async_buf_func() -> task_work_add()

For the IOPOLL setup, it should be protected by the ring mutex. Adding
the irq disable + lock for polling would be a nasty performance hit.

-- 
Jens Axboe

Re: [PATCH 2/8] io_uring: fix commit_cqring() locking in iopoll

[Pavel Begunkov]

On 30/06/2020 17:46, Jens Axboe wrote:
> On 6/30/20 8:36 AM, Pavel Begunkov wrote:
>> On 30/06/2020 17:04, Jens Axboe wrote:
>>> On 6/30/20 6:20 AM, Pavel Begunkov wrote:
>>>> Don't call io_commit_cqring() without holding the completion spinlock
>>>> in io_iopoll_complete(), it can race, e.g. with async request failing.
>>>
>>> Can you be more specific?
>>
>> io_iopoll_complete()
>> 	-> io_req_free_batch()
>> 		-> io_queue_next()
>> 			-> io_req_task_queue()
>> 				-> task_work_add()
>>
>> if this task_work_add() fails, it will be redirected to io-wq manager task
>> to do io_req_task_cancel() -> commit_cqring().
>>
>>
>> And probably something similar will happen if a request currently in io-wq
>> is retried with
>> io_rw_should_retry() -> io_async_buf_func() -> task_work_add()
> 
> For the IOPOLL setup, it should be protected by the ring mutex. Adding
> the irq disable + lock for polling would be a nasty performance hit.

Ok. For what it worth, it would be easier to accidentally screw in the future.
I'll try it out.


BTW, if you're going to cherry-pick patches from this series, just push them
into the branch. I'll check git.kernel.dk later and resend what's left.

-- 
Pavel Begunkov

Re: [PATCH 2/8] io_uring: fix commit_cqring() locking in iopoll

[Jens Axboe]

On 6/30/20 9:00 AM, Pavel Begunkov wrote:
> On 30/06/2020 17:46, Jens Axboe wrote:
>> On 6/30/20 8:36 AM, Pavel Begunkov wrote:
>>> On 30/06/2020 17:04, Jens Axboe wrote:
>>>> On 6/30/20 6:20 AM, Pavel Begunkov wrote:
>>>>> Don't call io_commit_cqring() without holding the completion spinlock
>>>>> in io_iopoll_complete(), it can race, e.g. with async request failing.
>>>>
>>>> Can you be more specific?
>>>
>>> io_iopoll_complete()
>>> 	-> io_req_free_batch()
>>> 		-> io_queue_next()
>>> 			-> io_req_task_queue()
>>> 				-> task_work_add()
>>>
>>> if this task_work_add() fails, it will be redirected to io-wq manager task
>>> to do io_req_task_cancel() -> commit_cqring().
>>>
>>>
>>> And probably something similar will happen if a request currently in io-wq
>>> is retried with
>>> io_rw_should_retry() -> io_async_buf_func() -> task_work_add()
>>
>> For the IOPOLL setup, it should be protected by the ring mutex. Adding
>> the irq disable + lock for polling would be a nasty performance hit.
> 
> Ok. For what it worth, it would be easier to accidentally screw in the future.
> I'll try it out.
> 
> 
> BTW, if you're going to cherry-pick patches from this series, just push them
> into the branch. I'll check git.kernel.dk later and resend what's left.

Yep done, I'll push it out.

-- 
Jens Axboe