Re: [PATCH] io_uring: REQ_F_FORCE_ASYNC prep done too late

[Pavel Begunkov <asml.silence@gmail.com>]

On 3/19/2020 6:51 AM, Jens Axboe wrote:
> A previous patch ensured that we always prepped requests that are
> forced async, but it did so too late in the process. This can result
> in 'sqe' already being NULL by the time we get to it:

Isn't it fixed by f1d96a8fcbbbb ("io_uring: NULL-deref for
IOSQE_{ASYNC,DRAIN}")? BTW, the same can happen with draining in
io_req_defer() -> io_req_defer_prep().

Can't look through your patches/RFC properly, but I will try do that
this weekends.


> BUG: kernel NULL pointer dereference, address: 0000000000000008
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> PGD 0 P4D 0 
> Oops: 0000 [#1] PREEMPT SMP
> CPU: 2 PID: 331 Comm: read-write Not tainted 5.6.0-rc5+ #5846
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> RIP: 0010:io_prep_rw+0x37/0x250
> Code: 41 89 d4 55 48 89 f5 53 48 8b 0f 48 89 fb 4c 8b 6f 50 48 8b 41 20 0f b7 00 66 25 00 f0 66 3d 00 80 75 07 81 4f 68 00 40 00 00 <48> 8b 45 08 48 83 f8 ff 48 89 43 08 0f 84 c6 01 00 00 8b 41 34 85
> RSP: 0018:ffffc900003ebce8 EFLAGS: 00010206
> RAX: 0000000000008000 RBX: ffff8881adf87b00 RCX: ffff8881b7db4f00
> RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff8881adf87b00
> RBP: 0000000000000000 R08: ffff8881adf87b88 R09: ffff8881adf87b88
> R10: 0000000000001000 R11: ffffea0006ddee08 R12: 0000000000000001
> R13: ffff8881b325c000 R14: 0000000000000000 R15: ffff8881b325c000
> FS:  00007f642a59e540(0000) GS:ffff8881b9d00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000008 CR3: 00000001b3674004 CR4: 00000000001606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  io_write_prep+0x1c/0x100
>  io_queue_sqe+0x8f/0x2a0
>  io_submit_sqes+0x450/0xa10
>  __do_sys_io_uring_enter+0x272/0x610
>  ? ksys_mmap_pgoff+0x15d/0x1f0
>  do_syscall_64+0x42/0x100
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7f642a4d0f8d
> Code: 00 c3 66 2e 0f 1f 84 00 00 00 00 00 90 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d d3 4e 0c 00 f7 d8 64 89 01 48
> 
> Fix this by ensuring we do prep at io_submit_sqe() time, where we
> know we still have the original sqe.
> 
> Fixes: 1118591ab883 ("io_uring: prep req when do IOSQE_ASYNC")
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> 
> ---
> 
> diff --git a/fs/io_uring.c b/fs/io_uring.c
> index e6049546e77c..ff35f5ac91ea 100644
> --- a/fs/io_uring.c
> +++ b/fs/io_uring.c
> @@ -5528,15 +5528,11 @@ static void io_queue_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe)
>  	ret = io_req_defer(req, sqe);
>  	if (ret) {
>  		if (ret != -EIOCBQUEUED) {
> -fail_req:
>  			io_cqring_add_event(req, ret);
>  			req_set_fail_links(req);
>  			io_double_put_req(req);
>  		}
>  	} else if (req->flags & REQ_F_FORCE_ASYNC) {
> -		ret = io_req_defer_prep(req, sqe);
> -		if (unlikely(ret < 0))
> -			goto fail_req;
>  		/*
>  		 * Never try inline submit of IOSQE_ASYNC is set, go straight
>  		 * to async execution.
> @@ -5650,12 +5646,21 @@ static bool io_submit_sqe(struct io_kiocb *req, const struct io_uring_sqe *sqe,
>  			req->flags |= REQ_F_IO_DRAIN;
>  			req->ctx->drain_next = 0;
>  		}
> +		if (sqe_flags & REQ_F_FORCE_ASYNC) {
> +			ret = io_req_defer_prep(req, sqe);
> +			if (ret < 0)
> +				goto err_req;
> +			/* prep done */
> +			sqe = NULL;
> +		}
>  		if (sqe_flags & (IOSQE_IO_LINK|IOSQE_IO_HARDLINK)) {
>  			req->flags |= REQ_F_LINK;
>  			INIT_LIST_HEAD(&req->link_list);
> -			ret = io_req_defer_prep(req, sqe);
> -			if (ret)
> -				req->flags |= REQ_F_FAIL_LINK;
> +			if (sqe) {
> +				ret = io_req_defer_prep(req, sqe);
> +				if (ret)
> +					req->flags |= REQ_F_FAIL_LINK;
> +			}
>  			*link = req;
>  		} else {
>  			io_queue_sqe(req, sqe);
> 

-- 
Pavel Begunkov