On 09/10/2020 15:12, Matthew Wilcox wrote:
> On Fri, Oct 09, 2020 at 02:10:49PM +0300, Pavel Begunkov wrote:
>>> kasan_report.cold+0x1f/0x37 mm/kasan/report.c:562
>>> xas_next_entry include/linux/xarray.h:1630 [inline]
>>> __io_uring_files_cancel+0x417/0x440 fs/io_uring.c:8681
>>> io_uring_files_cancel include/linux/io_uring.h:35 [inline]
>>> exit_files+0xe4/0x170 fs/file.c:456
>>> do_exit+0xae9/0x2930 kernel/exit.c:801
>>> do_group_exit+0x125/0x310 kernel/exit.c:903
>>> get_signal+0x428/0x1f00 kernel/signal.c:2757
>>> arch_do_signal+0x82/0x2470 arch/x86/kernel/signal.c:811
>>> exit_to_user_mode_loop kernel/entry/common.c:161 [inline]
>>> exit_to_user_mode_prepare+0x194/0x1f0 kernel/entry/common.c:192
>>> syscall_exit_to_user_mode+0x7a/0x2c0 kernel/entry/common.c:267
>>> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>>
>> It seems this fails on "node->shift" in xas_next_entry(), that would
>> mean that the node itself was freed while we're iterating on it.
>>
>> __io_uring_files_cancel() iterates with xas_next_entry() and creates
>> XA_STATE once by hand, but it also removes entries in the loop with
>> io_uring_del_task_file() -> xas_store(&xas, NULL); without updating
>> the iterating XA_STATE. Could it be the problem? See a diff below
>
> No, the problem is that the lock is dropped after calling
> xas_next_entry(), and at any point after calling xas_next_entry(),
> the node that it's pointing to can be freed.
Only the task itself clears/removes entries, others can only insert.
So, could it be freed even though there are no parallel erases?
>
> I don't think there's any benefit to using the advanced API here.
> Since io_uring_cancel_task_requests() can sleep, we have to drop the lock
> for each iteration around the loop, and so we have to walk from the top of the tree each time. So we may as well make this easy to read:
Thanks for looking into it, looks definitely better.
>
> @@ -8665,28 +8665,19 @@ static void io_uring_attempt_task_drop(struct file *file, bool exiting)
> void __io_uring_files_cancel(struct files_struct *files)
> {
> struct io_uring_task *tctx = current->io_uring;
> - XA_STATE(xas, &tctx->xa, 0);
> + struct file *file;
> + unsigned long index;
>
> /* make sure overflow events are dropped */
> tctx->in_idle = true;
>
> - do {
> - struct io_ring_ctx *ctx;
> - struct file *file;
> -
> - xas_lock(&xas);
> - file = xas_next_entry(&xas, ULONG_MAX);
> - xas_unlock(&xas);
> -
> - if (!file)
> - break;
> -
> - ctx = file->private_data;
> + xa_for_each(&tctx->xa, index, file) {
> + struct io_ring_ctx *ctx = file->private_data;
>
> io_uring_cancel_task_requests(ctx, files);
> if (files)
> io_uring_del_task_file(file);
> - } while (1);
> + }
> }
>
> static inline bool io_uring_task_idle(struct io_uring_task *tctx)
>
> I'll send a proper patch in a few minutes -- I'd like to neaten up a
> few of the other XArray uses.
>
--
Pavel Begunkov
