On 3/16/20 6:14 AM, Xiaoguang Wang wrote:
> hi,
>
> While diving into iouring file register/unregister/update codes, seems that
> there is one bug in __io_sqe_files_update():
> if (ref_switch)
> percpu_ref_switch_to_atomic(&data->refs, io_atomic_switch);
>
> The initial fixed_file_data's refs is 1, assume there are no requests
> to get/put this refs, and we firstly register 10 files and later update
> these 10 files, and no memory allocations fails, then above two line of
> codes in __io_sqe_files_update() will be called, before entering
> percpu_ref_switch_to_atomic(), the count of refs is still one, and
> |--> percpu_ref_switch_to_atomic
> |----> __percpu_ref_switch_mode
> |------> __percpu_ref_switch_to_atomic
> |-------- > percpu_ref_get(ref), # now the count of refs will be 2.
>
> a while later
> |--> percpu_ref_switch_to_atomic_rcu
> |----> percpu_ref_call_confirm_rcu
> |------ > confirm_switch(), # calls io_atomic_switch, note that the count of refs is 2.
> |------ > percpu_ref_put # drop one ref
>
> static void io_atomic_switch(struct percpu_ref *ref)
> {
> struct fixed_file_data *data;
>
> /*
> * Juggle reference to ensure we hit zero, if needed, so we can
> * switch back to percpu mode
> */
> data = container_of(ref, struct fixed_file_data, refs);
> percpu_ref_put(&data->refs);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> After this operation, the count of refs is 1 now, still not zero, so
> io_file_data_ref_zero won't be called, then io_ring_file_ref_flush()
> won't be called, this fixed_file_data's refs will always be in atomic mode,
> which is bad.
>
> percpu_ref_get(&data->refs);
> }
>
> To confirm this bug, I did a hack to kernel:
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -5812,7 +5812,10 @@ static bool io_queue_file_removal(struct fixed_file_data *data,
> * If we fail allocating the struct we need for doing async reomval
> * of this file, just punt to sync and wait for it.
> */
> + /*
> pfile = kzalloc(sizeof(*pfile), GFP_KERNEL);
> + */
> + pfile = NULL;
> if (!pfile) {
> pfile = &pfile_stack;
> pfile->done = &done;
> To simulate memory allocation failures, then run liburing/test/file-update,
>
> [lege@localhost test]$ sudo cat /proc/2091/stack
> [sudo] password for lege:
> [<0>] __io_sqe_files_update.isra.85+0x175/0x330
> [<0>] __io_uring_register+0x178/0xe20
> [<0>] __x64_sys_io_uring_register+0xa0/0x160
> [<0>] do_syscall_64+0x55/0x1b0
> [<0>] entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> (gdb) list * __io_sqe_files_update+0x175
> 0xffffffff812ec255 is in __io_sqe_files_update (fs/io_uring.c:5830).
> 5825 llist_add(&pfile->llist, &data->put_llist);
> 5826
> 5827 if (pfile == &pfile_stack) {
> 5828 percpu_ref_switch_to_atomic(&data->refs, io_atomic_switch);
> 5829 wait_for_completion(&done);
> 5830 flush_work(&data->ref_work);
> 5831 return false;
>
> file-update will always hang in wait_for_completion(&done), it's because
> io_ring_file_ref_flush never has a chance to run.
>
> I think how to fix this issue a while, doesn't find a elegant method yet.
> And applications may issue requests continuously, then fixed_file_data's refs
> may never have a chance to reach zero, refs will always be in atomic mode.
> Or the simplest method is to use percpu_ref per registered file :)
For the "oh crap I can't allocate data" stack path, I think the below
should fix it. Might not be a bad idea to re-think the live updates in
general, though.
diff --git a/fs/io_uring.c b/fs/io_uring.c
index b1fbc4424aa6..3f0c8291a17c 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -5612,10 +5612,12 @@ static void io_ring_file_ref_flush(struct fixed_file_data *data)
while ((node = llist_del_all(&data->put_llist)) != NULL) {
llist_for_each_entry_safe(pfile, tmp, node, llist) {
io_ring_file_put(data->ctx, pfile->file);
- if (pfile->done)
+ if (pfile->done) {
+ percpu_ref_get(&data->refs);
complete(pfile->done);
- else
+ } else {
kfree(pfile);
+ }
}
}
}
@@ -5830,6 +5836,7 @@ static bool io_queue_file_removal(struct fixed_file_data *data,
llist_add(&pfile->llist, &data->put_llist);
if (pfile == &pfile_stack) {
+ percpu_ref_put(&data->refs);
percpu_ref_switch_to_atomic(&data->refs, io_atomic_switch);
wait_for_completion(&done);
flush_work(&data->ref_work);
--
Jens Axboe
