* [PATCH 1/5] io_uring: fix punting req w/o grabbed env
2020-06-27 11:04 [PATCH 0/5] "task_work for links" fixes Pavel Begunkov
@ 2020-06-27 11:04 ` Pavel Begunkov
2020-06-27 11:04 ` [PATCH 2/5] io_uring: fix feeding io-wq with uninit reqs Pavel Begunkov
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Pavel Begunkov @ 2020-06-27 11:04 UTC (permalink / raw)
To: Jens Axboe, io-uring
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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/5] io_uring: fix feeding io-wq with uninit reqs
2020-06-27 11:04 [PATCH 0/5] "task_work for links" fixes Pavel Begunkov
2020-06-27 11:04 ` [PATCH 1/5] io_uring: fix punting req w/o grabbed env Pavel Begunkov
@ 2020-06-27 11:04 ` Pavel Begunkov
2020-06-27 11:04 ` [PATCH 3/5] io_uring: don't mark link's head for_async Pavel Begunkov
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Pavel Begunkov @ 2020-06-27 11:04 UTC (permalink / raw)
To: Jens Axboe, io-uring
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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/5] io_uring: don't mark link's head for_async
2020-06-27 11:04 [PATCH 0/5] "task_work for links" fixes Pavel Begunkov
2020-06-27 11:04 ` [PATCH 1/5] io_uring: fix punting req w/o grabbed env Pavel Begunkov
2020-06-27 11:04 ` [PATCH 2/5] io_uring: fix feeding io-wq with uninit reqs Pavel Begunkov
@ 2020-06-27 11:04 ` Pavel Begunkov
2020-06-27 11:04 ` [PATCH 4/5] io_uring: fix missing io_grab_files() Pavel Begunkov
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Pavel Begunkov @ 2020-06-27 11:04 UTC (permalink / raw)
To: Jens Axboe, io-uring
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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/5] io_uring: fix missing io_grab_files()
2020-06-27 11:04 [PATCH 0/5] "task_work for links" fixes Pavel Begunkov
` (2 preceding siblings ...)
2020-06-27 11:04 ` [PATCH 3/5] io_uring: don't mark link's head for_async Pavel Begunkov
@ 2020-06-27 11:04 ` Pavel Begunkov
2020-06-27 11:04 ` [PATCH 5/5] io_ring: fix req->work corruption Pavel Begunkov
2020-06-28 13:49 ` [PATCH 0/5] "task_work for links" fixes Jens Axboe
5 siblings, 0 replies; 12+ messages in thread
From: Pavel Begunkov @ 2020-06-27 11:04 UTC (permalink / raw)
To: Jens Axboe, io-uring
We won't have valid ring_fd, ring_file in task work,
Grab files early.
Signed-off-by: Pavel Begunkov <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 5/5] io_ring: fix req->work corruption
2020-06-27 11:04 [PATCH 0/5] "task_work for links" fixes Pavel Begunkov
` (3 preceding siblings ...)
2020-06-27 11:04 ` [PATCH 4/5] io_uring: fix missing io_grab_files() Pavel Begunkov
@ 2020-06-27 11:04 ` Pavel Begunkov
2020-06-28 13:49 ` [PATCH 0/5] "task_work for links" fixes Jens Axboe
5 siblings, 0 replies; 12+ messages in thread
From: Pavel Begunkov @ 2020-06-27 11:04 UTC (permalink / raw)
To: Jens Axboe, io-uring
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 <[email protected]>
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] "task_work for links" fixes
2020-06-27 11:04 [PATCH 0/5] "task_work for links" fixes Pavel Begunkov
` (4 preceding siblings ...)
2020-06-27 11:04 ` [PATCH 5/5] io_ring: fix req->work corruption Pavel Begunkov
@ 2020-06-28 13:49 ` Jens Axboe
2020-06-28 14:46 ` Pavel Begunkov
5 siblings, 1 reply; 12+ messages in thread
From: Jens Axboe @ 2020-06-28 13:49 UTC (permalink / raw)
To: Pavel Begunkov, io-uring
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] "task_work for links" fixes
2020-06-28 13:49 ` [PATCH 0/5] "task_work for links" fixes Jens Axboe
@ 2020-06-28 14:46 ` Pavel Begunkov
2020-06-29 10:21 ` Pavel Begunkov
0 siblings, 1 reply; 12+ messages in thread
From: Pavel Begunkov @ 2020-06-28 14:46 UTC (permalink / raw)
To: Jens Axboe, io-uring
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] "task_work for links" fixes
2020-06-28 14:46 ` Pavel Begunkov
@ 2020-06-29 10:21 ` Pavel Begunkov
2020-06-29 15:52 ` Jens Axboe
0 siblings, 1 reply; 12+ messages in thread
From: Pavel Begunkov @ 2020-06-29 10:21 UTC (permalink / raw)
To: Jens Axboe, io-uring
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] "task_work for links" fixes
2020-06-29 10:21 ` Pavel Begunkov
@ 2020-06-29 15:52 ` Jens Axboe
2020-06-29 16:32 ` Pavel Begunkov
0 siblings, 1 reply; 12+ messages in thread
From: Jens Axboe @ 2020-06-29 15:52 UTC (permalink / raw)
To: Pavel Begunkov, io-uring
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] "task_work for links" fixes
2020-06-29 15:52 ` Jens Axboe
@ 2020-06-29 16:32 ` Pavel Begunkov
2020-06-29 16:37 ` Jens Axboe
0 siblings, 1 reply; 12+ messages in thread
From: Pavel Begunkov @ 2020-06-29 16:32 UTC (permalink / raw)
To: Jens Axboe, io-uring
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/5] "task_work for links" fixes
2020-06-29 16:32 ` Pavel Begunkov
@ 2020-06-29 16:37 ` Jens Axboe
0 siblings, 0 replies; 12+ messages in thread
From: Jens Axboe @ 2020-06-29 16:37 UTC (permalink / raw)
To: Pavel Begunkov, io-uring
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
^ permalink raw reply [flat|nested] 12+ messages in thread