[PATCH 0/2] two 5.8 fixes

[PATCH 0/2] two 5.8 fixes

[Pavel Begunkov]

[2/2] is actually fixed in 5.9, but apparently it wasn't just a
speculation but rather an actual issue. It fixes locally, by moving
put out of lock, because don't see a reason why it's there.

Pavel Begunkov (2):
  io_uring: fix ->work corruption with poll_add
  io_uring: fix lockup in io_fail_links()

 fs/io_uring.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

-- 
2.24.0

[PATCH 1/2] io_uring: fix ->work corruption with poll_add

[Pavel Begunkov]

req->work might be already initialised by the time it gets into
__io_arm_poll_handler(), which will corrupt it be using fields that are
in an union with req->work. Luckily, the only side effect is missing
put_creds(). Clean req->work before going there.

Suggested-by: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 32b0064f806e..98e8079e67e7 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4658,6 +4658,10 @@ static int io_poll_add(struct io_kiocb *req)
 	struct io_poll_table ipt;
 	__poll_t mask;
 
+	/* ->work is in union with hash_node and others */
+	io_req_work_drop_env(req);
+	req->flags &= ~REQ_F_WORK_INITIALIZED;
+
 	INIT_HLIST_NODE(&req->hash_node);
 	INIT_LIST_HEAD(&req->list);
 	ipt.pt._qproc = io_poll_queue_proc;
-- 
2.24.0

[PATCH 2/2] io_uring: fix lockup in io_fail_links()

[Pavel Begunkov]

io_fail_links() doesn't consider REQ_F_COMP_LOCKED leading to nested
spin_lock(completion_lock) and lockup.

[  197.680409] rcu: INFO: rcu_preempt detected expedited stalls on
	CPUs/tasks: { 6-... } 18239 jiffies s: 1421 root: 0x40/.
[  197.680411] rcu: blocking rcu_node structures:
[  197.680412] Task dump for CPU 6:
[  197.680413] link-timeout    R  running task        0  1669
	1 0x8000008a
[  197.680414] Call Trace:
[  197.680420]  ? io_req_find_next+0xa0/0x200
[  197.680422]  ? io_put_req_find_next+0x2a/0x50
[  197.680423]  ? io_poll_task_func+0xcf/0x140
[  197.680425]  ? task_work_run+0x67/0xa0
[  197.680426]  ? do_exit+0x35d/0xb70
[  197.680429]  ? syscall_trace_enter+0x187/0x2c0
[  197.680430]  ? do_group_exit+0x43/0xa0
[  197.680448]  ? __x64_sys_exit_group+0x18/0x20
[  197.680450]  ? do_syscall_64+0x52/0xa0
[  197.680452]  ? entry_SYSCALL_64_after_hwframe+0x44/0xa9

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 98e8079e67e7..493e5047e67c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -4199,10 +4199,9 @@ static void io_poll_task_handler(struct io_kiocb *req, struct io_kiocb **nxt)
 
 	hash_del(&req->hash_node);
 	io_poll_complete(req, req->result, 0);
-	req->flags |= REQ_F_COMP_LOCKED;
-	io_put_req_find_next(req, nxt);
 	spin_unlock_irq(&ctx->completion_lock);
 
+	io_put_req_find_next(req, nxt);
 	io_cqring_ev_posted(ctx);
 }
 
-- 
2.24.0

Re: [PATCH 1/2] io_uring: fix ->work corruption with poll_add

[Pavel Begunkov]

On 24/07/2020 20:07, Pavel Begunkov wrote:
> req->work might be already initialised by the time it gets into
> __io_arm_poll_handler(), which will corrupt it be using fields that are
s/be using/by using/

Jens, could you please fold it in, if the patch would do? Or let me know
and I'll resend.

> in an union with req->work. Luckily, the only side effect is missing
> put_creds(). Clean req->work before going there.
> 
> Suggested-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/io_uring.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index 32b0064f806e..98e8079e67e7 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -4658,6 +4658,10 @@ static int io_poll_add(struct io_kiocb *req)
>  	struct io_poll_table ipt;
>  	__poll_t mask;
>  
> +	/* ->work is in union with hash_node and others */
> +	io_req_work_drop_env(req);
> +	req->flags &= ~REQ_F_WORK_INITIALIZED;
> +
>  	INIT_HLIST_NODE(&req->hash_node);
>  	INIT_LIST_HEAD(&req->list);
>  	ipt.pt._qproc = io_poll_queue_proc;
> 

-- 
Pavel Begunkov

Re: [PATCH 0/2] two 5.8 fixes

[Jens Axboe]

On 7/24/20 11:07 AM, Pavel Begunkov wrote:
> [2/2] is actually fixed in 5.9, but apparently it wasn't just a
> speculation but rather an actual issue. It fixes locally, by moving
> put out of lock, because don't see a reason why it's there.

Thanks, I applied with the fixed spelling, and shuffled things around
a bit.

-- 
Jens Axboe