On 11/3/21 18:21, Tadeusz Struk wrote:
Note: this applies to 5.10 stable only. It doesn't trigger on anything above 5.10 as the code there has been substantially reworked. This also doesn't apply to any stable kernel below 5.10 afaict.
Syzbot found a bug: KASAN: invalid-free in io_dismantle_req https://syzkaller.appspot.com/bug?id=123d9a852fc88ba573ffcb2dbcf4f9576c3b055...
The test submits bunch of io_uring writes and exits, which then triggers uring_task_cancel() and io_put_identity(), which in some corner cases, tries to free a static identity. This causes a panic as shown in the trace below:
BUG: KASAN: double-free or invalid-free in kfree+0xd5/0x310 CPU: 0 PID: 4618 Comm: repro Not tainted 5.10.76-05281-g4944ec82ebb9-dirty #17 Call Trace: dump_stack_lvl+0x1b2/0x21b print_address_description+0x8d/0x3b0 kasan_report_invalid_free+0x58/0x130 ____kasan_slab_free+0x14b/0x170 __kasan_slab_free+0x11/0x20 slab_free_freelist_hook+0xcc/0x1a0 kfree+0xd5/0x310 io_dismantle_req+0x9b0/0xd90 io_do_iopoll+0x13a4/0x23e0 io_iopoll_try_reap_events+0x116/0x290 io_uring_cancel_task_requests+0x197d/0x1ee0 io_uring_flush+0x170/0x6d0 filp_close+0xb0/0x150 put_files_struct+0x1d4/0x350 exit_files+0x80/0xa0 do_exit+0x6d9/0x2390 do_group_exit+0x16a/0x2d0 get_signal+0x133e/0x1f80 arch_do_signal+0x7b/0x610 exit_to_user_mode_prepare+0xaa/0xe0 syscall_exit_to_user_mode+0x24/0x40 do_syscall_64+0x3d/0x70 entry_SYSCALL_64_after_hwframe+0x44/0xa9
Allocated by task 4611: ____kasan_kmalloc+0xcd/0x100 __kasan_kmalloc+0x9/0x10 kmem_cache_alloc_trace+0x208/0x390 io_uring_alloc_task_context+0x57/0x550 io_uring_add_task_file+0x1f7/0x290 io_uring_create+0x2195/0x3490 __x64_sys_io_uring_setup+0x1bf/0x280 do_syscall_64+0x31/0x70 entry_SYSCALL_64_after_hwframe+0x44/0xa9
The buggy address belongs to the object at ffff88810732b500 which belongs to the cache kmalloc-192 of size 192 The buggy address is located 88 bytes inside of 192-byte region [ffff88810732b500, ffff88810732b5c0) Kernel panic - not syncing: panic_on_warn set ...
This issue bisected to this commit: commit 186725a80c4e ("io_uring: fix skipping disabling sqo on exec")
Simple reverting the offending commit doesn't work as it hits some other, related issues like:
/* sqo_dead check is for when this happens after cancellation */ WARN_ON_ONCE(ctx->sqo_task == current && !ctx->sqo_dead && !xa_load(&tctx->xa, (unsigned long)file));
------------[ cut here ]------------ WARNING: CPU: 1 PID: 5622 at fs/io_uring.c:8960 io_uring_flush+0x5bc/0x6d0 Modules linked in: CPU: 1 PID: 5622 Comm: repro Not tainted 5.10.76-05281-g4944ec82ebb9-dirty #16 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-6.fc35 04/01/2014 RIP: 0010:io_uring_flush+0x5bc/0x6d0 Call Trace: filp_close+0xb0/0x150 put_files_struct+0x1d4/0x350 reset_files_struct+0x88/0xa0 bprm_execve+0x7f2/0x9f0 do_execveat_common+0x46f/0x5d0 __x64_sys_execve+0x92/0xb0 do_syscall_64+0x31/0x70 entry_SYSCALL_64_after_hwframe+0x44/0xa9
Changing __io_uring_task_cancel() to call io_disable_sqo_submit() directly, as the comment suggests, only if __io_uring_files_cancel() is not executed seems to fix the issue.
Cc: Jens Axboe axboe@kernel.dk Cc: Alexander Viro viro@zeniv.linux.org.uk Cc: io-uring@vger.kernel.org Cc: linux-fsdevel@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: stable@vger.kernel.org Reported-by: syzbot+6055980d041c8ac23307@syzkaller.appspotmail.com Signed-off-by: Tadeusz Struk tadeusz.struk@linaro.org
fs/io_uring.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/fs/io_uring.c b/fs/io_uring.c index 0736487165da..fcf9ffe9b209 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -8882,20 +8882,18 @@ void __io_uring_task_cancel(void) struct io_uring_task *tctx = current->io_uring; DEFINE_WAIT(wait); s64 inflight;
- int canceled = 0;
/* make sure overflow events are dropped */ atomic_inc(&tctx->in_idle);
- /* trigger io_disable_sqo_submit() */
- if (tctx->sqpoll)
__io_uring_files_cancel(NULL);- do { /* read completions before cancelations */ inflight = tctx_inflight(tctx); if (!inflight) break; __io_uring_files_cancel(NULL);
canceled = 1;prepare_to_wait(&tctx->wait, &wait, TASK_UNINTERRUPTIBLE); @@ -8909,6 +8907,21 @@ void __io_uring_task_cancel(void) finish_wait(&tctx->wait, &wait); } while (1);
- /*
* trigger io_disable_sqo_submit()* if not already done by __io_uring_files_cancel()*/- if (tctx->sqpoll && !canceled) {
struct file *file;unsigned long index;xa_for_each(&tctx->xa, index, file) {struct io_ring_ctx *ctx = file->private_data;io_disable_sqo_submit(ctx);}- }
- atomic_dec(&tctx->in_idle);
io_uring_remove_task_files(tctx);
Hi, Any comments on this one? It needs to be ACK'ed by the maintainer before it is applied to 5.10 stable.