[PATCH 0/5] "task_work for links" fixes

[PATCH 0/5] "task_work for links" fixes

[Pavel Begunkov]

All but [3/5] are different segfault fixes for
c40f63790ec9 ("io_uring: use task_work for links if possible")


Pavel Begunkov (5):
  io_uring: fix punting req w/o grabbed env
  io_uring: fix feeding io-wq with uninit reqs
  io_uring: don't mark link's head for_async
  io_uring: fix missing io_grab_files()
  io_ring: fix req->work corruption

 fs/io_uring.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

-- 
2.24.0

[PATCH 1/5] io_uring: fix punting req w/o grabbed env

[Pavel Begunkov]

It's not enough to check for REQ_F_WORK_INITIALIZED and punt async
assuming that io_req_work_grab_env() was called, it may have been
not. E.g. io_close_prep() and personality path set the flag without
further async init.

As a quick fix, always pass next work through io_req_task_queue().

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index af4d7a5c49f4..e10aeae8cc52 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1766,12 +1766,8 @@ static void io_free_req(struct io_kiocb *req)
 	io_req_find_next(req, &nxt);
 	__io_free_req(req);
 
-	if (nxt) {
-		if (nxt->flags & REQ_F_WORK_INITIALIZED)
-			io_queue_async_work(nxt);
-		else
-			io_req_task_queue(nxt);
-	}
+	if (nxt)
+		io_req_task_queue(nxt);
 }
 
 /*
-- 
2.24.0

[PATCH 2/5] io_uring: fix feeding io-wq with uninit reqs

[Pavel Begunkov]

io_steal_work() can't be sure that @nxt have req->work
properly set, so we can't pass it to io-wq as is.

A dirty quick fix -- drag it through io_req_task_queue(),
and always return NULL from io_steal_work().

e.g.
[   50.770161] BUG: kernel NULL pointer dereference, address: 00000000
[   50.770164] #PF: supervisor write access in kernel mode
[   50.770164] #PF: error_code(0x0002) - not-present page
[   50.770168] CPU: 1 PID: 1448 Comm: io_wqe_worker-0 Tainted: G
	I       5.8.0-rc2-00035-g2237d76530eb-dirty #494
[   50.770172] RIP: 0010:override_creds+0x19/0x30
...
[   50.770183]  io_worker_handle_work+0x25c/0x430
[   50.770185]  io_wqe_worker+0x2a0/0x350
[   50.770190]  kthread+0x136/0x180
[   50.770194]  ret_from_fork+0x22/0x30

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index e10aeae8cc52..b577d6f50cbc 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -1791,7 +1791,7 @@ static void io_put_req(struct io_kiocb *req)
 
 static struct io_wq_work *io_steal_work(struct io_kiocb *req)
 {
-	struct io_kiocb *link, *nxt = NULL;
+	struct io_kiocb *nxt = NULL;
 
 	/*
 	 * A ref is owned by io-wq in which context we're. So, if that's the
@@ -1808,10 +1808,15 @@ static struct io_wq_work *io_steal_work(struct io_kiocb *req)
 	if ((nxt->flags & REQ_F_ISREG) && io_op_defs[nxt->opcode].hash_reg_file)
 		io_wq_hash_work(&nxt->work, file_inode(nxt->file));
 
-	link = io_prep_linked_timeout(nxt);
-	if (link)
-		nxt->flags |= REQ_F_QUEUE_TIMEOUT;
-	return &nxt->work;
+	io_req_task_queue(nxt);
+	/*
+	 * If we're going to return actual work, here should be timeout prep:
+	 *
+	 * link = io_prep_linked_timeout(nxt);
+	 * if (link)
+	 *	nxt->flags |= REQ_F_QUEUE_TIMEOUT;
+	 */
+	return NULL;
 }
 
 /*
-- 
2.24.0

[PATCH 3/5] io_uring: don't mark link's head for_async

[Pavel Begunkov]

No reason to mark a head of a link as for-async in
io_req_defer_prep(). grab_env(), etc.that will be done
further during submission if neccessary.

Mark for_async=false saving extra grab_env() in many cases.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index b577d6f50cbc..ca486bb5444a 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -6086,7 +6086,7 @@ static int io_submit_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
 			if (io_alloc_async_ctx(req))
 				return -EAGAIN;
 
-			ret = io_req_defer_prep(req, sqe, true);
+			ret = io_req_defer_prep(req, sqe, false);
 			if (ret)
 				req->flags |= REQ_F_FAIL_LINK;
 			*link = req;
-- 
2.24.0

[PATCH 4/5] io_uring: fix missing io_grab_files()

[Pavel Begunkov]

We won't have valid ring_fd, ring_file in task work,
Grab files early.

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index ca486bb5444a..e2b5f51ebb30 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5264,15 +5264,15 @@ static int io_req_defer_prep(struct io_kiocb *req,
 	if (!sqe)
 		return 0;
 
-	if (for_async || (req->flags & REQ_F_WORK_INITIALIZED)) {
+	if (io_op_defs[req->opcode].file_table) {
 		io_req_init_async(req);
+		ret = io_grab_files(req);
+		if (unlikely(ret))
+			return ret;
+	}
 
-		if (io_op_defs[req->opcode].file_table) {
-			ret = io_grab_files(req);
-			if (unlikely(ret))
-				return ret;
-		}
-
+	if (for_async || (req->flags & REQ_F_WORK_INITIALIZED)) {
+		io_req_init_async(req);
 		io_req_work_grab_env(req, &io_op_defs[req->opcode]);
 	}
 
-- 
2.24.0

[PATCH 5/5] io_ring: fix req->work corruption

[Pavel Begunkov]

req->work and req->task_work are in union, so io_req_task_queue()
screws everything that was in work. De-union them for now.

[  704.367253] BUG: unable to handle page fault for address:
	ffffffffaf7330d0
[  704.367256] #PF: supervisor write access in kernel mode
[  704.367256] #PF: error_code(0x0003) - permissions violation
[  704.367261] CPU: 6 PID: 1654 Comm: io_wqe_worker-0 Tainted: G
I       5.8.0-rc2-00038-ge28d0bdc4863-dirty #498
[  704.367265] RIP: 0010:_raw_spin_lock+0x1e/0x36
...
[  704.367276]  __alloc_fd+0x35/0x150
[  704.367279]  __get_unused_fd_flags+0x25/0x30
[  704.367280]  io_openat2+0xcb/0x1b0
[  704.367283]  io_issue_sqe+0x36a/0x1320
[  704.367294]  io_wq_submit_work+0x58/0x160
[  704.367295]  io_worker_handle_work+0x2a3/0x430
[  704.367296]  io_wqe_worker+0x2a0/0x350
[  704.367301]  kthread+0x136/0x180
[  704.367304]  ret_from_fork+0x22/0x30

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

diff --git a/fs/io_uring.c b/fs/io_uring.c
index e2b5f51ebb30..bf236ba10601 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -668,12 +668,12 @@ struct io_kiocb {
 		 * restore the work, if needed.
 		 */
 		struct {
-			struct callback_head	task_work;
 			struct hlist_node	hash_node;
 			struct async_poll	*apoll;
 		};
 		struct io_wq_work	work;
 	};
+	struct callback_head	task_work;
 };
 
 #define IO_IOPOLL_BATCH			8
-- 
2.24.0

Re: [PATCH 0/5] "task_work for links" fixes

[Jens Axboe]

On 6/27/20 5:04 AM, Pavel Begunkov wrote:
> All but [3/5] are different segfault fixes for
> c40f63790ec9 ("io_uring: use task_work for links if possible")

Looks reasonable, too bad about the task_work moving out of the
union, but I agree there's no other nice way to avoid this. BTW,
fwiw, I've moved that to the head of the series.

-- 
Jens Axboe

Re: [PATCH 0/5] "task_work for links" fixes

[Pavel Begunkov]

On 28/06/2020 16:49, Jens Axboe wrote:
> On 6/27/20 5:04 AM, Pavel Begunkov wrote:
>> All but [3/5] are different segfault fixes for
>> c40f63790ec9 ("io_uring: use task_work for links if possible")
> 
> Looks reasonable, too bad about the task_work moving out of the
> union, but I agree there's no other nice way to avoid this. BTW,
> fwiw, I've moved that to the head of the series.

I think I'll move it back, but that would need more work to be
done. I've described the idea in the other thread.

-- 
Pavel Begunkov

Re: [PATCH 0/5] "task_work for links" fixes

[Pavel Begunkov]

On 28/06/2020 17:46, Pavel Begunkov wrote:
> On 28/06/2020 16:49, Jens Axboe wrote:
>> On 6/27/20 5:04 AM, Pavel Begunkov wrote:
>>> All but [3/5] are different segfault fixes for
>>> c40f63790ec9 ("io_uring: use task_work for links if possible")
>>
>> Looks reasonable, too bad about the task_work moving out of the
>> union, but I agree there's no other nice way to avoid this. BTW,
>> fwiw, I've moved that to the head of the series.
> 
> I think I'll move it back, but that would need more work to be
> done. I've described the idea in the other thread.

BTW, do you know any way to do grab_files() from task_work context?
The problem is that nobody sets ctx->ring_{fd,file} there. Using stale
values won't do, as ring_fd can be of another process at that point.


-- 
Pavel Begunkov

Re: [PATCH 0/5] "task_work for links" fixes

[Jens Axboe]

On 6/29/20 4:21 AM, Pavel Begunkov wrote:
> On 28/06/2020 17:46, Pavel Begunkov wrote:
>> On 28/06/2020 16:49, Jens Axboe wrote:
>>> On 6/27/20 5:04 AM, Pavel Begunkov wrote:
>>>> All but [3/5] are different segfault fixes for
>>>> c40f63790ec9 ("io_uring: use task_work for links if possible")
>>>
>>> Looks reasonable, too bad about the task_work moving out of the
>>> union, but I agree there's no other nice way to avoid this. BTW,
>>> fwiw, I've moved that to the head of the series.
>>
>> I think I'll move it back, but that would need more work to be
>> done. I've described the idea in the other thread.
> 
> BTW, do you know any way to do grab_files() from task_work context?
> The problem is that nobody sets ctx->ring_{fd,file} there. Using stale
> values won't do, as ring_fd can be of another process at that point.

We probably have to have them grabbed up-front. Which should be easy
enough to do now, since task_work and work are no longer in a union.

-- 
Jens Axboe

Re: [PATCH 0/5] "task_work for links" fixes

[Pavel Begunkov]

On 29/06/2020 18:52, Jens Axboe wrote:
> On 6/29/20 4:21 AM, Pavel Begunkov wrote:
>> On 28/06/2020 17:46, Pavel Begunkov wrote:
>>> On 28/06/2020 16:49, Jens Axboe wrote:
>>>> On 6/27/20 5:04 AM, Pavel Begunkov wrote:
>>>>> All but [3/5] are different segfault fixes for
>>>>> c40f63790ec9 ("io_uring: use task_work for links if possible")
>>>>
>>>> Looks reasonable, too bad about the task_work moving out of the
>>>> union, but I agree there's no other nice way to avoid this. BTW,
>>>> fwiw, I've moved that to the head of the series.
>>>
>>> I think I'll move it back, but that would need more work to be
>>> done. I've described the idea in the other thread.
>>
>> BTW, do you know any way to do grab_files() from task_work context?
>> The problem is that nobody sets ctx->ring_{fd,file} there. Using stale
>> values won't do, as ring_fd can be of another process at that point.
> 
> We probably have to have them grabbed up-front. Which should be easy
> enough to do now, since task_work and work are no longer in a union.

Yep, and it's how it's done. Just looking how to handle req.work better.
e.g. if we can grab_files() from task_work, then it's one step from
moving back req.work into union + totally removing memcpy(work, apoll)
from io_arm_poll_handler().

-- 
Pavel Begunkov

Re: [PATCH 0/5] "task_work for links" fixes

[Jens Axboe]

On 6/29/20 10:32 AM, Pavel Begunkov wrote:
> On 29/06/2020 18:52, Jens Axboe wrote:
>> On 6/29/20 4:21 AM, Pavel Begunkov wrote:
>>> On 28/06/2020 17:46, Pavel Begunkov wrote:
>>>> On 28/06/2020 16:49, Jens Axboe wrote:
>>>>> On 6/27/20 5:04 AM, Pavel Begunkov wrote:
>>>>>> All but [3/5] are different segfault fixes for
>>>>>> c40f63790ec9 ("io_uring: use task_work for links if possible")
>>>>>
>>>>> Looks reasonable, too bad about the task_work moving out of the
>>>>> union, but I agree there's no other nice way to avoid this. BTW,
>>>>> fwiw, I've moved that to the head of the series.
>>>>
>>>> I think I'll move it back, but that would need more work to be
>>>> done. I've described the idea in the other thread.
>>>
>>> BTW, do you know any way to do grab_files() from task_work context?
>>> The problem is that nobody sets ctx->ring_{fd,file} there. Using stale
>>> values won't do, as ring_fd can be of another process at that point.
>>
>> We probably have to have them grabbed up-front. Which should be easy
>> enough to do now, since task_work and work are no longer in a union.
> 
> Yep, and it's how it's done. Just looking how to handle req.work better.
> e.g. if we can grab_files() from task_work, then it's one step from
> moving back req.work into union + totally removing memcpy(work, apoll)
> from io_arm_poll_handler().

Indeed, and both of those are very worthy goals fwiw. If at all possible,
it'd be nicer to get rid of the restriction of having to check ring_fd
and file, but that doesn't seem possible without making the general
io_ring_enter() system call more expensive.

-- 
Jens Axboe