* Re: [PATCH] io_uring: always let io_iopoll_complete() complete polled io.
2020-12-02 11:31 [PATCH] io_uring: always let io_iopoll_complete() complete polled io Xiaoguang Wang
@ 2020-12-02 17:00 ` Pavel Begunkov
2020-12-02 17:09 ` Pavel Begunkov
2020-12-03 2:30 ` Joseph Qi
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Pavel Begunkov @ 2020-12-02 17:00 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: axboe, joseph.qi
On 02/12/2020 11:31, Xiaoguang Wang wrote:
> Abaci Fuzz reported a double-free or invalid-free BUG in io_commit_cqring():
> [ 95.504842] BUG: KASAN: double-free or invalid-free in io_commit_cqring+0x3ec/0x8e0
> [ 95.505921]
> [ 95.506225] CPU: 0 PID: 4037 Comm: io_wqe_worker-0 Tainted: G B
> W 5.10.0-rc5+ #1
> [ 95.507434] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> [ 95.508248] Call Trace:
> [ 95.508683] dump_stack+0x107/0x163
> [ 95.509323] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.509982] print_address_description.constprop.0+0x3e/0x60
> [ 95.510814] ? vprintk_func+0x98/0x140
> [ 95.511399] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.512036] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.512733] kasan_report_invalid_free+0x51/0x80
> [ 95.513431] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.514047] __kasan_slab_free+0x141/0x160
> [ 95.514699] kfree+0xd1/0x390
> [ 95.515182] io_commit_cqring+0x3ec/0x8e0
> [ 95.515799] __io_req_complete.part.0+0x64/0x90
> [ 95.516483] io_wq_submit_work+0x1fa/0x260
> [ 95.517117] io_worker_handle_work+0xeac/0x1c00
> [ 95.517828] io_wqe_worker+0xc94/0x11a0
> [ 95.518438] ? io_worker_handle_work+0x1c00/0x1c00
> [ 95.519151] ? __kthread_parkme+0x11d/0x1d0
> [ 95.519806] ? io_worker_handle_work+0x1c00/0x1c00
> [ 95.520512] ? io_worker_handle_work+0x1c00/0x1c00
> [ 95.521211] kthread+0x396/0x470
> [ 95.521727] ? _raw_spin_unlock_irq+0x24/0x30
> [ 95.522380] ? kthread_mod_delayed_work+0x180/0x180
> [ 95.523108] ret_from_fork+0x22/0x30
> [ 95.523684]
> [ 95.523985] Allocated by task 4035:
> [ 95.524543] kasan_save_stack+0x1b/0x40
> [ 95.525136] __kasan_kmalloc.constprop.0+0xc2/0xd0
> [ 95.525882] kmem_cache_alloc_trace+0x17b/0x310
> [ 95.533930] io_queue_sqe+0x225/0xcb0
> [ 95.534505] io_submit_sqes+0x1768/0x25f0
> [ 95.535164] __x64_sys_io_uring_enter+0x89e/0xd10
> [ 95.535900] do_syscall_64+0x33/0x40
> [ 95.536465] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 95.537199]
> [ 95.537505] Freed by task 4035:
> [ 95.538003] kasan_save_stack+0x1b/0x40
> [ 95.538599] kasan_set_track+0x1c/0x30
> [ 95.539177] kasan_set_free_info+0x1b/0x30
> [ 95.539798] __kasan_slab_free+0x112/0x160
> [ 95.540427] kfree+0xd1/0x390
> [ 95.540910] io_commit_cqring+0x3ec/0x8e0
> [ 95.541516] io_iopoll_complete+0x914/0x1390
> [ 95.542150] io_do_iopoll+0x580/0x700
> [ 95.542724] io_iopoll_try_reap_events.part.0+0x108/0x200
> [ 95.543512] io_ring_ctx_wait_and_kill+0x118/0x340
> [ 95.544206] io_uring_release+0x43/0x50
> [ 95.544791] __fput+0x28d/0x940
> [ 95.545291] task_work_run+0xea/0x1b0
> [ 95.545873] do_exit+0xb6a/0x2c60
> [ 95.546400] do_group_exit+0x12a/0x320
> [ 95.546967] __x64_sys_exit_group+0x3f/0x50
> [ 95.547605] do_syscall_64+0x33/0x40
> [ 95.548155] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> The reason is that once we got a non EAGAIN error in io_wq_submit_work(),
> we'll complete req by calling io_req_complete(), which will hold completion_lock
> to call io_commit_cqring(), but for polled io, io_iopoll_complete() won't
> hold completion_lock to call io_commit_cqring(), then there maybe concurrent
> access to ctx->defer_list, double free may happen.
>
> To fix this bug, we always let io_iopoll_complete() complete polled io.
It makes sense if it got there though means of REQ_F_FORCE_ASYNC or as a linked,
but a thing I'm afraid of is going twice through the end section of io_issue_sqe()
(i.e. io_iopoll_req_issued). Shouldn't happen though.
Reviewed-by: Pavel Begunkov <[email protected]>
>
> Reported-by: Abaci Fuzz <[email protected]>
> Signed-off-by: Xiaoguang Wang <[email protected]>
> ---
> fs/io_uring.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index a8c136a..901ca67 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6074,8 +6074,19 @@ static struct io_wq_work *io_wq_submit_work(struct io_wq_work *work)
> }
>
> if (ret) {
> - req_set_fail_links(req);
> - io_req_complete(req, ret);
> + /*
> + * io_iopoll_complete() does not hold completion_lock to complete
> + * polled io, so here for polled io, just mark it done and still let
> + * io_iopoll_complete() complete it.
> + */
> + if (req->ctx->flags & IORING_SETUP_IOPOLL) {
> + struct kiocb *kiocb = &req->rw.kiocb;
> +
> + kiocb_done(kiocb, ret, NULL);
> + } else {
> + req_set_fail_links(req);
> + io_req_complete(req, ret);
> + }
> }
>
> return io_steal_work(req);
>
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] io_uring: always let io_iopoll_complete() complete polled io.
2020-12-02 17:00 ` Pavel Begunkov
@ 2020-12-02 17:09 ` Pavel Begunkov
0 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2020-12-02 17:09 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: axboe, joseph.qi
On 02/12/2020 17:00, Pavel Begunkov wrote:
> On 02/12/2020 11:31, Xiaoguang Wang wrote:
>> Abaci Fuzz reported a double-free or invalid-free BUG in io_commit_cqring():
>> [ 95.504842] BUG: KASAN: double-free or invalid-free in io_commit_cqring+0x3ec/0x8e0
>> [ 95.505921]
>> [ 95.506225] CPU: 0 PID: 4037 Comm: io_wqe_worker-0 Tainted: G B
>> W 5.10.0-rc5+ #1
>> [ 95.507434] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
>> [ 95.508248] Call Trace:
>> [ 95.508683] dump_stack+0x107/0x163
>> [ 95.509323] ? io_commit_cqring+0x3ec/0x8e0
>> [ 95.509982] print_address_description.constprop.0+0x3e/0x60
>> [ 95.510814] ? vprintk_func+0x98/0x140
>> [ 95.511399] ? io_commit_cqring+0x3ec/0x8e0
>> [ 95.512036] ? io_commit_cqring+0x3ec/0x8e0
>> [ 95.512733] kasan_report_invalid_free+0x51/0x80
>> [ 95.513431] ? io_commit_cqring+0x3ec/0x8e0
>> [ 95.514047] __kasan_slab_free+0x141/0x160
>> [ 95.514699] kfree+0xd1/0x390
>> [ 95.515182] io_commit_cqring+0x3ec/0x8e0
>> [ 95.515799] __io_req_complete.part.0+0x64/0x90
>> [ 95.516483] io_wq_submit_work+0x1fa/0x260
>> [ 95.517117] io_worker_handle_work+0xeac/0x1c00
>> [ 95.517828] io_wqe_worker+0xc94/0x11a0
>> [ 95.518438] ? io_worker_handle_work+0x1c00/0x1c00
>> [ 95.519151] ? __kthread_parkme+0x11d/0x1d0
>> [ 95.519806] ? io_worker_handle_work+0x1c00/0x1c00
>> [ 95.520512] ? io_worker_handle_work+0x1c00/0x1c00
>> [ 95.521211] kthread+0x396/0x470
>> [ 95.521727] ? _raw_spin_unlock_irq+0x24/0x30
>> [ 95.522380] ? kthread_mod_delayed_work+0x180/0x180
>> [ 95.523108] ret_from_fork+0x22/0x30
>> [ 95.523684]
>> [ 95.523985] Allocated by task 4035:
>> [ 95.524543] kasan_save_stack+0x1b/0x40
>> [ 95.525136] __kasan_kmalloc.constprop.0+0xc2/0xd0
>> [ 95.525882] kmem_cache_alloc_trace+0x17b/0x310
>> [ 95.533930] io_queue_sqe+0x225/0xcb0
>> [ 95.534505] io_submit_sqes+0x1768/0x25f0
>> [ 95.535164] __x64_sys_io_uring_enter+0x89e/0xd10
>> [ 95.535900] do_syscall_64+0x33/0x40
>> [ 95.536465] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [ 95.537199]
>> [ 95.537505] Freed by task 4035:
>> [ 95.538003] kasan_save_stack+0x1b/0x40
>> [ 95.538599] kasan_set_track+0x1c/0x30
>> [ 95.539177] kasan_set_free_info+0x1b/0x30
>> [ 95.539798] __kasan_slab_free+0x112/0x160
>> [ 95.540427] kfree+0xd1/0x390
>> [ 95.540910] io_commit_cqring+0x3ec/0x8e0
>> [ 95.541516] io_iopoll_complete+0x914/0x1390
>> [ 95.542150] io_do_iopoll+0x580/0x700
>> [ 95.542724] io_iopoll_try_reap_events.part.0+0x108/0x200
>> [ 95.543512] io_ring_ctx_wait_and_kill+0x118/0x340
>> [ 95.544206] io_uring_release+0x43/0x50
>> [ 95.544791] __fput+0x28d/0x940
>> [ 95.545291] task_work_run+0xea/0x1b0
>> [ 95.545873] do_exit+0xb6a/0x2c60
>> [ 95.546400] do_group_exit+0x12a/0x320
>> [ 95.546967] __x64_sys_exit_group+0x3f/0x50
>> [ 95.547605] do_syscall_64+0x33/0x40
>> [ 95.548155] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> The reason is that once we got a non EAGAIN error in io_wq_submit_work(),
>> we'll complete req by calling io_req_complete(), which will hold completion_lock
>> to call io_commit_cqring(), but for polled io, io_iopoll_complete() won't
>> hold completion_lock to call io_commit_cqring(), then there maybe concurrent
>> access to ctx->defer_list, double free may happen.
>>
>> To fix this bug, we always let io_iopoll_complete() complete polled io.
This one can use a test by the way. E.g. sending a bunch of iopoll requests,
both generic and REQ_F_FORCE_ASYNC, and expect it not to fail.
>
> It makes sense if it got there though means of REQ_F_FORCE_ASYNC or as a linked,
> but a thing I'm afraid of is going twice through the end section of io_issue_sqe()
> (i.e. io_iopoll_req_issued). Shouldn't happen though.
>
> Reviewed-by: Pavel Begunkov <[email protected]>
>
>>
>> Reported-by: Abaci Fuzz <[email protected]>
>> Signed-off-by: Xiaoguang Wang <[email protected]>
>> ---
>> fs/io_uring.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index a8c136a..901ca67 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -6074,8 +6074,19 @@ static struct io_wq_work *io_wq_submit_work(struct io_wq_work *work)
>> }
>>
>> if (ret) {
>> - req_set_fail_links(req);
>> - io_req_complete(req, ret);
>> + /*
>> + * io_iopoll_complete() does not hold completion_lock to complete
>> + * polled io, so here for polled io, just mark it done and still let
>> + * io_iopoll_complete() complete it.
>> + */
>> + if (req->ctx->flags & IORING_SETUP_IOPOLL) {
>> + struct kiocb *kiocb = &req->rw.kiocb;
>> +
>> + kiocb_done(kiocb, ret, NULL);
>> + } else {
>> + req_set_fail_links(req);
>> + io_req_complete(req, ret);
>> + }
>> }
>>
>> return io_steal_work(req);
>>
>
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] io_uring: always let io_iopoll_complete() complete polled io.
2020-12-02 11:31 [PATCH] io_uring: always let io_iopoll_complete() complete polled io Xiaoguang Wang
2020-12-02 17:00 ` Pavel Begunkov
@ 2020-12-03 2:30 ` Joseph Qi
2020-12-04 20:31 ` Pavel Begunkov
2020-12-06 22:25 ` Pavel Begunkov
2020-12-11 12:29 ` Pavel Begunkov
3 siblings, 1 reply; 10+ messages in thread
From: Joseph Qi @ 2020-12-03 2:30 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: axboe
On 12/2/20 7:31 PM, Xiaoguang Wang wrote:
> Abaci Fuzz reported a double-free or invalid-free BUG in io_commit_cqring():
> [ 95.504842] BUG: KASAN: double-free or invalid-free in io_commit_cqring+0x3ec/0x8e0
> [ 95.505921]
> [ 95.506225] CPU: 0 PID: 4037 Comm: io_wqe_worker-0 Tainted: G B
> W 5.10.0-rc5+ #1
> [ 95.507434] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> [ 95.508248] Call Trace:
> [ 95.508683] dump_stack+0x107/0x163
> [ 95.509323] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.509982] print_address_description.constprop.0+0x3e/0x60
> [ 95.510814] ? vprintk_func+0x98/0x140
> [ 95.511399] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.512036] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.512733] kasan_report_invalid_free+0x51/0x80
> [ 95.513431] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.514047] __kasan_slab_free+0x141/0x160
> [ 95.514699] kfree+0xd1/0x390
> [ 95.515182] io_commit_cqring+0x3ec/0x8e0
> [ 95.515799] __io_req_complete.part.0+0x64/0x90
> [ 95.516483] io_wq_submit_work+0x1fa/0x260
> [ 95.517117] io_worker_handle_work+0xeac/0x1c00
> [ 95.517828] io_wqe_worker+0xc94/0x11a0
> [ 95.518438] ? io_worker_handle_work+0x1c00/0x1c00
> [ 95.519151] ? __kthread_parkme+0x11d/0x1d0
> [ 95.519806] ? io_worker_handle_work+0x1c00/0x1c00
> [ 95.520512] ? io_worker_handle_work+0x1c00/0x1c00
> [ 95.521211] kthread+0x396/0x470
> [ 95.521727] ? _raw_spin_unlock_irq+0x24/0x30
> [ 95.522380] ? kthread_mod_delayed_work+0x180/0x180
> [ 95.523108] ret_from_fork+0x22/0x30
> [ 95.523684]
> [ 95.523985] Allocated by task 4035:
> [ 95.524543] kasan_save_stack+0x1b/0x40
> [ 95.525136] __kasan_kmalloc.constprop.0+0xc2/0xd0
> [ 95.525882] kmem_cache_alloc_trace+0x17b/0x310
> [ 95.533930] io_queue_sqe+0x225/0xcb0
> [ 95.534505] io_submit_sqes+0x1768/0x25f0
> [ 95.535164] __x64_sys_io_uring_enter+0x89e/0xd10
> [ 95.535900] do_syscall_64+0x33/0x40
> [ 95.536465] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 95.537199]
> [ 95.537505] Freed by task 4035:
> [ 95.538003] kasan_save_stack+0x1b/0x40
> [ 95.538599] kasan_set_track+0x1c/0x30
> [ 95.539177] kasan_set_free_info+0x1b/0x30
> [ 95.539798] __kasan_slab_free+0x112/0x160
> [ 95.540427] kfree+0xd1/0x390
> [ 95.540910] io_commit_cqring+0x3ec/0x8e0
> [ 95.541516] io_iopoll_complete+0x914/0x1390
> [ 95.542150] io_do_iopoll+0x580/0x700
> [ 95.542724] io_iopoll_try_reap_events.part.0+0x108/0x200
> [ 95.543512] io_ring_ctx_wait_and_kill+0x118/0x340
> [ 95.544206] io_uring_release+0x43/0x50
> [ 95.544791] __fput+0x28d/0x940
> [ 95.545291] task_work_run+0xea/0x1b0
> [ 95.545873] do_exit+0xb6a/0x2c60
> [ 95.546400] do_group_exit+0x12a/0x320
> [ 95.546967] __x64_sys_exit_group+0x3f/0x50
> [ 95.547605] do_syscall_64+0x33/0x40
> [ 95.548155] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> The reason is that once we got a non EAGAIN error in io_wq_submit_work(),
> we'll complete req by calling io_req_complete(), which will hold completion_lock
> to call io_commit_cqring(), but for polled io, io_iopoll_complete() won't
> hold completion_lock to call io_commit_cqring(), then there maybe concurrent
> access to ctx->defer_list, double free may happen.
>
> To fix this bug, we always let io_iopoll_complete() complete polled io.
>
> Reported-by: Abaci Fuzz <[email protected]>
> Signed-off-by: Xiaoguang Wang <[email protected]>
> ---
> fs/io_uring.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index a8c136a..901ca67 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6074,8 +6074,19 @@ static struct io_wq_work *io_wq_submit_work(struct io_wq_work *work)
> }
>
> if (ret) {
> - req_set_fail_links(req);
> - io_req_complete(req, ret);
> + /*
> + * io_iopoll_complete() does not hold completion_lock to complete
> + * polled io, so here for polled io, just mark it done and still let
> + * io_iopoll_complete() complete it.
> + */
> + if (req->ctx->flags & IORING_SETUP_IOPOLL) {
> + struct kiocb *kiocb = &req->rw.kiocb;
> +
> + kiocb_done(kiocb, ret, NULL);
> + } else {
> + req_set_fail_links(req);
> + io_req_complete(req, ret);
> + }
> }
>
> return io_steal_work(req);
>
This patch can also fix another BUG I'm looking at:
[ 61.359713] BUG: KASAN: double-free or invalid-free in io_dismantle_req+0x938/0xf40
...
[ 61.409315] refcount_t: underflow; use-after-free.
[ 61.410261] WARNING: CPU: 1 PID: 1022 at lib/refcount.c:28 refcount_warn_saturate+0x266/0x2a0
...
It blames io_put_identity() has been called more than once and then
identity->count is underflow.
Reviewed-by: Joseph Qi <[email protected]>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] io_uring: always let io_iopoll_complete() complete polled io.
2020-12-03 2:30 ` Joseph Qi
@ 2020-12-04 20:31 ` Pavel Begunkov
[not found] ` <[email protected]>
0 siblings, 1 reply; 10+ messages in thread
From: Pavel Begunkov @ 2020-12-04 20:31 UTC (permalink / raw)
To: Joseph Qi, Xiaoguang Wang, io-uring; +Cc: axboe
On 03/12/2020 02:30, Joseph Qi wrote:
> This patch can also fix another BUG I'm looking at:
>
> [ 61.359713] BUG: KASAN: double-free or invalid-free in io_dismantle_req+0x938/0xf40
> ...
> [ 61.409315] refcount_t: underflow; use-after-free.
> [ 61.410261] WARNING: CPU: 1 PID: 1022 at lib/refcount.c:28 refcount_warn_saturate+0x266/0x2a0
> ...
>
> It blames io_put_identity() has been called more than once and then
> identity->count is underflow.
Joseph, regarding your double-free
1. did you figure out how exactly this happens?
2. is it appears consistently so you can be sure that it's fixed
3. do you have a reproducer?
4. can you paste a full log of this BUG? (not cutting the stacktrace)
There are problems left even with this patch applied, but I need to
confirm which bug you saw.
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] io_uring: always let io_iopoll_complete() complete polled io.
2020-12-02 11:31 [PATCH] io_uring: always let io_iopoll_complete() complete polled io Xiaoguang Wang
2020-12-02 17:00 ` Pavel Begunkov
2020-12-03 2:30 ` Joseph Qi
@ 2020-12-06 22:25 ` Pavel Begunkov
2020-12-11 12:29 ` Pavel Begunkov
3 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2020-12-06 22:25 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: axboe, joseph.qi
On 02/12/2020 11:31, Xiaoguang Wang wrote:
[...]
> The reason is that once we got a non EAGAIN error in io_wq_submit_work(),
> we'll complete req by calling io_req_complete(), which will hold completion_lock
> to call io_commit_cqring(), but for polled io, io_iopoll_complete() won't
> hold completion_lock to call io_commit_cqring(), then there maybe concurrent
> access to ctx->defer_list, double free may happen.
>
> To fix this bug, we always let io_iopoll_complete() complete polled io.
I took this one into a series.
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] io_uring: always let io_iopoll_complete() complete polled io.
2020-12-02 11:31 [PATCH] io_uring: always let io_iopoll_complete() complete polled io Xiaoguang Wang
` (2 preceding siblings ...)
2020-12-06 22:25 ` Pavel Begunkov
@ 2020-12-11 12:29 ` Pavel Begunkov
2020-12-11 14:59 ` Xiaoguang Wang
3 siblings, 1 reply; 10+ messages in thread
From: Pavel Begunkov @ 2020-12-11 12:29 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: axboe, joseph.qi
On 02/12/2020 11:31, Xiaoguang Wang wrote:
> Abaci Fuzz reported a double-free or invalid-free BUG in io_commit_cqring():
> [ 95.504842] BUG: KASAN: double-free or invalid-free in io_commit_cqring+0x3ec/0x8e0
> [ 95.505921]
> [ 95.506225] CPU: 0 PID: 4037 Comm: io_wqe_worker-0 Tainted: G B
> W 5.10.0-rc5+ #1
> [ 95.507434] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
> [ 95.508248] Call Trace:
> [ 95.508683] dump_stack+0x107/0x163
> [ 95.509323] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.509982] print_address_description.constprop.0+0x3e/0x60
> [ 95.510814] ? vprintk_func+0x98/0x140
> [ 95.511399] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.512036] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.512733] kasan_report_invalid_free+0x51/0x80
> [ 95.513431] ? io_commit_cqring+0x3ec/0x8e0
> [ 95.514047] __kasan_slab_free+0x141/0x160
> [ 95.514699] kfree+0xd1/0x390
> [ 95.515182] io_commit_cqring+0x3ec/0x8e0
> [ 95.515799] __io_req_complete.part.0+0x64/0x90
> [ 95.516483] io_wq_submit_work+0x1fa/0x260
> [ 95.517117] io_worker_handle_work+0xeac/0x1c00
> [ 95.517828] io_wqe_worker+0xc94/0x11a0
> [ 95.518438] ? io_worker_handle_work+0x1c00/0x1c00
> [ 95.519151] ? __kthread_parkme+0x11d/0x1d0
> [ 95.519806] ? io_worker_handle_work+0x1c00/0x1c00
> [ 95.520512] ? io_worker_handle_work+0x1c00/0x1c00
> [ 95.521211] kthread+0x396/0x470
> [ 95.521727] ? _raw_spin_unlock_irq+0x24/0x30
> [ 95.522380] ? kthread_mod_delayed_work+0x180/0x180
> [ 95.523108] ret_from_fork+0x22/0x30
> [ 95.523684]
> [ 95.523985] Allocated by task 4035:
> [ 95.524543] kasan_save_stack+0x1b/0x40
> [ 95.525136] __kasan_kmalloc.constprop.0+0xc2/0xd0
> [ 95.525882] kmem_cache_alloc_trace+0x17b/0x310
> [ 95.533930] io_queue_sqe+0x225/0xcb0
> [ 95.534505] io_submit_sqes+0x1768/0x25f0
> [ 95.535164] __x64_sys_io_uring_enter+0x89e/0xd10
> [ 95.535900] do_syscall_64+0x33/0x40
> [ 95.536465] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 95.537199]
> [ 95.537505] Freed by task 4035:
> [ 95.538003] kasan_save_stack+0x1b/0x40
> [ 95.538599] kasan_set_track+0x1c/0x30
> [ 95.539177] kasan_set_free_info+0x1b/0x30
> [ 95.539798] __kasan_slab_free+0x112/0x160
> [ 95.540427] kfree+0xd1/0x390
> [ 95.540910] io_commit_cqring+0x3ec/0x8e0
> [ 95.541516] io_iopoll_complete+0x914/0x1390
> [ 95.542150] io_do_iopoll+0x580/0x700
> [ 95.542724] io_iopoll_try_reap_events.part.0+0x108/0x200
> [ 95.543512] io_ring_ctx_wait_and_kill+0x118/0x340
> [ 95.544206] io_uring_release+0x43/0x50
> [ 95.544791] __fput+0x28d/0x940
> [ 95.545291] task_work_run+0xea/0x1b0
> [ 95.545873] do_exit+0xb6a/0x2c60
> [ 95.546400] do_group_exit+0x12a/0x320
> [ 95.546967] __x64_sys_exit_group+0x3f/0x50
> [ 95.547605] do_syscall_64+0x33/0x40
> [ 95.548155] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> The reason is that once we got a non EAGAIN error in io_wq_submit_work(),
> we'll complete req by calling io_req_complete(), which will hold completion_lock
> to call io_commit_cqring(), but for polled io, io_iopoll_complete() won't
> hold completion_lock to call io_commit_cqring(), then there maybe concurrent
> access to ctx->defer_list, double free may happen.
>
> To fix this bug, we always let io_iopoll_complete() complete polled io.
>
> Reported-by: Abaci Fuzz <[email protected]>
> Signed-off-by: Xiaoguang Wang <[email protected]>
> ---
> fs/io_uring.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index a8c136a..901ca67 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -6074,8 +6074,19 @@ static struct io_wq_work *io_wq_submit_work(struct io_wq_work *work)
> }
>
> if (ret) {
> - req_set_fail_links(req);
> - io_req_complete(req, ret);
> + /*
> + * io_iopoll_complete() does not hold completion_lock to complete
> + * polled io, so here for polled io, just mark it done and still let
> + * io_iopoll_complete() complete it.
> + */
> + if (req->ctx->flags & IORING_SETUP_IOPOLL) {
> + struct kiocb *kiocb = &req->rw.kiocb;
Also remembered that IOPOLL apart from rw can do buf reg/unreg requests, so
it won't work for them. Could you fix it?
> +
> + kiocb_done(kiocb, ret, NULL);
> + } else {
> + req_set_fail_links(req);
> + io_req_complete(req, ret);
> + }
> }
>
> return io_steal_work(req);
>
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] io_uring: always let io_iopoll_complete() complete polled io.
2020-12-11 12:29 ` Pavel Begunkov
@ 2020-12-11 14:59 ` Xiaoguang Wang
2020-12-11 18:20 ` Pavel Begunkov
0 siblings, 1 reply; 10+ messages in thread
From: Xiaoguang Wang @ 2020-12-11 14:59 UTC (permalink / raw)
To: Pavel Begunkov, io-uring; +Cc: axboe, joseph.qi
hi Pavel,
> On 02/12/2020 11:31, Xiaoguang Wang wrote:
>> Abaci Fuzz reported a double-free or invalid-free BUG in io_commit_cqring():
>> [ 95.504842] BUG: KASAN: double-free or invalid-free in io_commit_cqring+0x3ec/0x8e0
>> [ 95.505921]
>> [ 95.506225] CPU: 0 PID: 4037 Comm: io_wqe_worker-0 Tainted: G B
>> W 5.10.0-rc5+ #1
>> [ 95.507434] Hardware name: Red Hat KVM, BIOS 0.5.1 01/01/2011
>> [ 95.508248] Call Trace:
>> [ 95.508683] dump_stack+0x107/0x163
>> [ 95.509323] ? io_commit_cqring+0x3ec/0x8e0
>> [ 95.509982] print_address_description.constprop.0+0x3e/0x60
>> [ 95.510814] ? vprintk_func+0x98/0x140
>> [ 95.511399] ? io_commit_cqring+0x3ec/0x8e0
>> [ 95.512036] ? io_commit_cqring+0x3ec/0x8e0
>> [ 95.512733] kasan_report_invalid_free+0x51/0x80
>> [ 95.513431] ? io_commit_cqring+0x3ec/0x8e0
>> [ 95.514047] __kasan_slab_free+0x141/0x160
>> [ 95.514699] kfree+0xd1/0x390
>> [ 95.515182] io_commit_cqring+0x3ec/0x8e0
>> [ 95.515799] __io_req_complete.part.0+0x64/0x90
>> [ 95.516483] io_wq_submit_work+0x1fa/0x260
>> [ 95.517117] io_worker_handle_work+0xeac/0x1c00
>> [ 95.517828] io_wqe_worker+0xc94/0x11a0
>> [ 95.518438] ? io_worker_handle_work+0x1c00/0x1c00
>> [ 95.519151] ? __kthread_parkme+0x11d/0x1d0
>> [ 95.519806] ? io_worker_handle_work+0x1c00/0x1c00
>> [ 95.520512] ? io_worker_handle_work+0x1c00/0x1c00
>> [ 95.521211] kthread+0x396/0x470
>> [ 95.521727] ? _raw_spin_unlock_irq+0x24/0x30
>> [ 95.522380] ? kthread_mod_delayed_work+0x180/0x180
>> [ 95.523108] ret_from_fork+0x22/0x30
>> [ 95.523684]
>> [ 95.523985] Allocated by task 4035:
>> [ 95.524543] kasan_save_stack+0x1b/0x40
>> [ 95.525136] __kasan_kmalloc.constprop.0+0xc2/0xd0
>> [ 95.525882] kmem_cache_alloc_trace+0x17b/0x310
>> [ 95.533930] io_queue_sqe+0x225/0xcb0
>> [ 95.534505] io_submit_sqes+0x1768/0x25f0
>> [ 95.535164] __x64_sys_io_uring_enter+0x89e/0xd10
>> [ 95.535900] do_syscall_64+0x33/0x40
>> [ 95.536465] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>> [ 95.537199]
>> [ 95.537505] Freed by task 4035:
>> [ 95.538003] kasan_save_stack+0x1b/0x40
>> [ 95.538599] kasan_set_track+0x1c/0x30
>> [ 95.539177] kasan_set_free_info+0x1b/0x30
>> [ 95.539798] __kasan_slab_free+0x112/0x160
>> [ 95.540427] kfree+0xd1/0x390
>> [ 95.540910] io_commit_cqring+0x3ec/0x8e0
>> [ 95.541516] io_iopoll_complete+0x914/0x1390
>> [ 95.542150] io_do_iopoll+0x580/0x700
>> [ 95.542724] io_iopoll_try_reap_events.part.0+0x108/0x200
>> [ 95.543512] io_ring_ctx_wait_and_kill+0x118/0x340
>> [ 95.544206] io_uring_release+0x43/0x50
>> [ 95.544791] __fput+0x28d/0x940
>> [ 95.545291] task_work_run+0xea/0x1b0
>> [ 95.545873] do_exit+0xb6a/0x2c60
>> [ 95.546400] do_group_exit+0x12a/0x320
>> [ 95.546967] __x64_sys_exit_group+0x3f/0x50
>> [ 95.547605] do_syscall_64+0x33/0x40
>> [ 95.548155] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> The reason is that once we got a non EAGAIN error in io_wq_submit_work(),
>> we'll complete req by calling io_req_complete(), which will hold completion_lock
>> to call io_commit_cqring(), but for polled io, io_iopoll_complete() won't
>> hold completion_lock to call io_commit_cqring(), then there maybe concurrent
>> access to ctx->defer_list, double free may happen.
>>
>> To fix this bug, we always let io_iopoll_complete() complete polled io.
>>
>> Reported-by: Abaci Fuzz <[email protected]>
>> Signed-off-by: Xiaoguang Wang <[email protected]>
>> ---
>> fs/io_uring.c | 15 +++++++++++++--
>> 1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/io_uring.c b/fs/io_uring.c
>> index a8c136a..901ca67 100644
>> --- a/fs/io_uring.c
>> +++ b/fs/io_uring.c
>> @@ -6074,8 +6074,19 @@ static struct io_wq_work *io_wq_submit_work(struct io_wq_work *work)
>> }
>>
>> if (ret) {
>> - req_set_fail_links(req);
>> - io_req_complete(req, ret);
>> + /*
>> + * io_iopoll_complete() does not hold completion_lock to complete
>> + * polled io, so here for polled io, just mark it done and still let
>> + * io_iopoll_complete() complete it.
>> + */
>> + if (req->ctx->flags & IORING_SETUP_IOPOLL) {
>> + struct kiocb *kiocb = &req->rw.kiocb;
>
> Also remembered that IOPOLL apart from rw can do buf reg/unreg requests, so
> it won't work for them. Could you fix it?
Sorry for late, I had saw you and jens' discussion in previous mail. I was just
busy recently, did't have time to look into this issue. Now I have time to handle
it, will have a look at it tomorrow, thanks.
Regards,
Xiaoguang Wang
>
>> +
>> + kiocb_done(kiocb, ret, NULL);
>> + } else {
>> + req_set_fail_links(req);
>> + io_req_complete(req, ret);
>> + }
>> }
>>
>> return io_steal_work(req);
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] io_uring: always let io_iopoll_complete() complete polled io.
2020-12-11 14:59 ` Xiaoguang Wang
@ 2020-12-11 18:20 ` Pavel Begunkov
0 siblings, 0 replies; 10+ messages in thread
From: Pavel Begunkov @ 2020-12-11 18:20 UTC (permalink / raw)
To: Xiaoguang Wang, io-uring; +Cc: axboe, joseph.qi
On 11/12/2020 14:59, Xiaoguang Wang wrote:
> Sorry for late, I had saw you and jens' discussion in previous mail. I was just
> busy recently, did't have time to look into this issue. Now I have time to handle
> it, will have a look at it tomorrow, thanks.
All cool, as Jens dropped it and decided to do that for 5.11 we're not in hurry,
just wanted to point out stuff to look after! You could also look for a similar
problem with io_cqring_events() if you're interested.
One more thing to ponder is failing -EAGAIN'ed requests in io_iopoll_complete()
if NOWAIT is set.
--
Pavel Begunkov
^ permalink raw reply [flat|nested] 10+ messages in thread