On 8/30/24 15:12, Jens Axboe wrote: > On 8/29/24 4:32 PM, Bernd Schubert wrote: >> Wanted to send out a new series today, >> >> https://github.com/bsbernd/linux/tree/fuse-uring-for-6.10-rfc3-without-mmap >> >> but then just noticed a tear down issue. >> >> 1525.905504] KASAN: null-ptr-deref in range [0x00000000000001a0-0x00000000000001a7] >> [ 1525.910431] CPU: 15 PID: 183 Comm: kworker/15:1 Tainted: G O 6.10.0+ #48 >> [ 1525.916449] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 >> [ 1525.922470] Workqueue: events io_fallback_req_func >> [ 1525.925840] RIP: 0010:__lock_acquire+0x74/0x7b80 >> [ 1525.929010] Code: 89 bc 24 80 00 00 00 0f 85 1c 5f 00 00 83 3d 6e 80 b0 02 00 0f 84 1d 12 00 00 83 3d 65 c7 67 02 00 74 27 48 89 f8 48 c1 e8 03 <42> 80 3c 30 00 74 0d e8 50 44 42 00 48 8b bc 24 80 00 00 00 48 c7 >> [ 1525.942211] RSP: 0018:ffff88810b2af490 EFLAGS: 00010002 >> [ 1525.945672] RAX: 0000000000000034 RBX: 0000000000000000 RCX: 0000000000000001 >> [ 1525.950421] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000000000001a0 >> [ 1525.955200] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000000 >> [ 1525.959979] R10: dffffc0000000000 R11: fffffbfff07b1cbe R12: 0000000000000000 >> [ 1525.964252] R13: 0000000000000001 R14: dffffc0000000000 R15: 0000000000000001 >> [ 1525.968225] FS: 0000000000000000(0000) GS:ffff88875b200000(0000) knlGS:0000000000000000 >> [ 1525.973932] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> [ 1525.976694] CR2: 00005555b6a381f0 CR3: 000000012f5f1000 CR4: 00000000000006f0 >> [ 1525.980030] Call Trace: >> [ 1525.981371] <TASK> >> [ 1525.982567] ? __die_body+0x66/0xb0 >> [ 1525.984376] ? die_addr+0xc1/0x100 >> [ 1525.986111] ? exc_general_protection+0x1c6/0x330 >> [ 1525.988401] ? asm_exc_general_protection+0x22/0x30 >> [ 1525.990864] ? __lock_acquire+0x74/0x7b80 >> [ 1525.992901] ? mark_lock+0x9f/0x360 >> [ 1525.994635] ? __lock_acquire+0x1420/0x7b80 >> [ 1525.996629] ? attach_entity_load_avg+0x47d/0x550 >> [ 1525.998765] ? hlock_conflict+0x5a/0x1f0 >> [ 1526.000515] ? __bfs+0x2dc/0x5a0 >> [ 1526.001993] lock_acquire+0x1fb/0x3d0 >> [ 1526.004727] ? gup_fast_fallback+0x13f/0x1d80 >> [ 1526.006586] ? gup_fast_fallback+0x13f/0x1d80 >> [ 1526.008412] gup_fast_fallback+0x158/0x1d80 >> [ 1526.010170] ? gup_fast_fallback+0x13f/0x1d80 >> [ 1526.011999] ? __lock_acquire+0x2b07/0x7b80 >> [ 1526.013793] __iov_iter_get_pages_alloc+0x36e/0x980 >> [ 1526.015876] ? do_raw_spin_unlock+0x5a/0x8a0 >> [ 1526.017734] iov_iter_get_pages2+0x56/0x70 >> [ 1526.019491] fuse_copy_fill+0x48e/0x980 [fuse] >> [ 1526.021400] fuse_copy_args+0x174/0x6a0 [fuse] >> [ 1526.023199] fuse_uring_prepare_send+0x319/0x6c0 [fuse] >> [ 1526.025178] fuse_uring_send_req_in_task+0x42/0x100 [fuse] >> [ 1526.027163] io_fallback_req_func+0xb4/0x170 >> [ 1526.028737] ? process_scheduled_works+0x75b/0x1160 >> [ 1526.030445] process_scheduled_works+0x85c/0x1160 >> [ 1526.032073] worker_thread+0x8ba/0xce0 >> [ 1526.033388] kthread+0x23e/0x2b0 >> [ 1526.035404] ? pr_cont_work_flush+0x290/0x290 >> [ 1526.036958] ? kthread_blkcg+0xa0/0xa0 >> [ 1526.038321] ret_from_fork+0x30/0x60 >> [ 1526.039600] ? kthread_blkcg+0xa0/0xa0 >> [ 1526.040942] ret_from_fork_asm+0x11/0x20 >> [ 1526.042353] </TASK> >> >> >> We probably need to call iov_iter_get_pages2() immediately >> on submitting the buffer from fuse server and not only when needed. >> I had planned to do that as optimization later on, I think >> it is also needed to avoid io_uring_cmd_complete_in_task(). > > I think you do, but it's not really what's wrong here - fallback work is > being invoked as the ring is being torn down, either directly or because > the task is exiting. Your task_work should check if this is the case, > and just do -ECANCELED for this case rather than attempt to execute the > work. Most task_work doesn't do much outside of post a completion, but > yours seems complex in that attempts to map pages as well, for example. > In any case, regardless of whether you move the gup to the actual issue > side of things (which I think you should), then you'd want something > ala: > > if (req->task != current) > don't issue, -ECANCELED > > in your task_work. Thanks a lot for your help Jens! I'm a bit confused, doesn't this belong into __io_uring_cmd_do_in_task then? Because my task_work_cb function (passed to io_uring_cmd_complete_in_task) doesn't even have the request. I'm going to test this in a bit diff --git a/io_uring/uring_cmd.c b/io_uring/uring_cmd.c index 21ac5fb2d5f0..c06b9fcff48f 100644 --- a/io_uring/uring_cmd.c +++ b/io_uring/uring_cmd.c @@ -120,6 +120,11 @@ static void io_uring_cmd_work(struct io_kiocb *req, struct io_tw_state *ts) { struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd); + if (req->task != current) { + /* don't issue, -ECANCELED */ + return; + } + /* task_work executor checks the deffered list completion */ ioucmd->task_work_cb(ioucmd, IO_URING_F_COMPLETE_DEFER); } > >> The part I don't like here is that with mmap we had a complex >> initialization - but then either it worked or did not. No exceptions >> at IO time. And run time was just a copy into the buffer. >> Without mmap initialization is much simpler, but now complexity shifts >> to IO time. > > I'll take a look at your code. But I'd say just fix the missing check > above and send out what you have, it's much easier to iterate on the > list rather than poking at patches in some git branch somewhere. > I'm almost through updating it, will send something out definitely today. I will just keep the last patch that pins user buffer pages on top of the series - will avoid all the rebasing. Thanks, Bernd
