[PATCH] io_uring: Fix leaking double_put()

[PATCH] io_uring: Fix leaking double_put()

[Pavel Begunkov]

io_double_put_req() may be called for a request with a link (see
io_req_defer(req)), and so can leak it in case of an error, as
__io_free_req() doesn't handle links.

Fixes: 78e19bbef38362ceb ("io_uring: pass in io_kiocb to fill/add CQ
handlers")

Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
---
 fs/io_uring.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/io_uring.c b/fs/io_uring.c
index 18711d45b994..82f139ebdbbc 100644
--- a/fs/io_uring.c
+++ b/fs/io_uring.c
@@ -376,7 +376,6 @@ static void io_wq_submit_work(struct io_wq_work **workptr);
 static void io_cqring_fill_event(struct io_kiocb *req, long res);
 static void __io_free_req(struct io_kiocb *req);
 static void io_put_req(struct io_kiocb *req);
-static void io_double_put_req(struct io_kiocb *req);
 
 static struct kmem_cache *req_cachep;
 
@@ -930,7 +929,8 @@ static void io_fail_links(struct io_kiocb *req)
 			io_link_cancel_timeout(link);
 		} else {
 			io_cqring_fill_event(link, -ECANCELED);
-			io_double_put_req(link);
+			if (refcount_sub_and_test(2, &req->refs))
+				__io_free_req(req);
 		}
 	}
 
@@ -1007,7 +1007,7 @@ static void io_double_put_req(struct io_kiocb *req)
 {
 	/* drop both submit and complete references */
 	if (refcount_sub_and_test(2, &req->refs))
-		__io_free_req(req);
+		io_free_req(req);
 }
 
 static unsigned io_cqring_events(struct io_ring_ctx *ctx, bool noflush)
@@ -2830,6 +2830,8 @@ static int io_queue_sqe(struct io_kiocb *req)
 	if (ret) {
 		if (ret != -EIOCBQUEUED) {
 			io_cqring_add_event(req, ret);
+			if (req->flags & REQ_F_LINK)
+				req->flags |= REQ_F_FAIL_LINK;
 			io_double_put_req(req);
 		}
 		return 0;
@@ -2857,6 +2859,8 @@ static int io_queue_link_head(struct io_kiocb *req, struct io_kiocb *shadow)
 	if (ret) {
 		if (ret != -EIOCBQUEUED) {
 			io_cqring_add_event(req, ret);
+			if (req->flags & REQ_F_LINK)
+				req->flags |= REQ_F_FAIL_LINK;
 			io_double_put_req(req);
 			__io_free_req(shadow);
 			return 0;
@@ -2903,6 +2907,8 @@ static void io_submit_sqe(struct io_kiocb *req, struct io_submit_state *state,
 	if (unlikely(ret)) {
 err_req:
 		io_cqring_add_event(req, ret);
+		if (req->flags & REQ_F_LINK)
+			req->flags |= REQ_F_FAIL_LINK;
 		io_double_put_req(req);
 		return;
 	}
-- 
2.24.0

Re: [PATCH] io_uring: Fix leaking double_put()

[Jens Axboe]

On 11/12/19 12:17 AM, Pavel Begunkov wrote:
> io_double_put_req() may be called for a request with a link (see
> io_req_defer(req)), and so can leak it in case of an error, as
> __io_free_req() doesn't handle links.
> 
> Fixes: 78e19bbef38362ceb ("io_uring: pass in io_kiocb to fill/add CQ
> handlers")

This blows up the 'link' test from the liburing regression suite:

[   20.007180] refcount_t: underflow; use-after-free.
[   20.008562] WARNING: CPU: 0 PID: 278 at lib/refcount.c:190 refcount_sub_and_test_checked+0xf3/0x100
[   20.010784] Modules linked in:
[   20.011565] CPU: 0 PID: 278 Comm: link Not tainted 5.4.0-rc5+ #3490
[   20.013112] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.10.2-1ubuntu1 04/01/2014
[   20.015540] RIP: 0010:refcount_sub_and_test_checked+0xf3/0x100
[   20.017312] Code: 5d 41 5c 41 5d 41 5e c3 eb db 44 0f b6 35 cb a3 3a 01 45 84 f6 75 cb 48 c7 c7 e0 55 1a 82 c6 05 b8 a3 3a 01 01 e8 30 92 99 ff <0f> 0b eb b7 66 0f 1f 84 00 00 00 00 00 48 89 fe bf 01 00 00 00 e9
[   20.021244] RSP: 0018:ffff8881005f7af8 EFLAGS: 00010086
[   20.022159] RAX: 0000000000000000 RBX: 00000000fffffffe RCX: 0000000000000000
[   20.023419] RDX: 0000000000000001 RSI: 0000000000000008 RDI: ffffed10200bef55
[   20.024775] RBP: ffff8881003f2e54 R08: 0000000000000001 R09: ffffed10218c3ee1
[   20.025743] R10: ffffed10218c3ee0 R11: ffff88810c61f707 R12: 0000000000000002
[   20.026645] R13: 1ffff110200bef60 R14: 0000000000000000 R15: ffff8881003f2e40
[   20.027546] FS:  00007f5f0be74540(0000) GS:ffff88810c600000(0000) knlGS:0000000000000000
[   20.028700] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   20.029726] CR2: 00007f5f0bceabd0 CR3: 00000001072ab001 CR4: 00000000001606f0
[   20.030880] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   20.032127] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   20.033457] Call Trace:
[   20.033999]  ? refcount_dec_if_one+0x90/0x90
[   20.034861]  ? debug_lockdep_rcu_enabled.part.0+0x16/0x30
[   20.035583]  ? io_cqring_fill_event+0x11d/0x330
[   20.036035]  io_free_req_find_next+0x20c/0x350
[   20.036631]  __io_queue_sqe+0x2db/0x9c0
[   20.037219]  ? io_wq_submit_work+0x220/0x220
[   20.037795]  ? io_req_defer+0x6c/0x3d0
[   20.038404]  ? rcu_read_lock_sched_held+0x81/0xb0
[   20.039045]  io_submit_sqes+0x69e/0xee0
[   20.039521]  ? io_queue_link_head+0x2c0/0x2c0
[   20.040109]  ? mutex_lock_io_nested+0xbd0/0xbd0
[   20.040730]  ? find_held_lock+0x85/0xa0
[   20.041238]  ? __x64_sys_io_uring_enter+0x1be/0x660
[   20.041611]  ? lock_downgrade+0x310/0x310
[   20.041911]  ? lock_acquire+0xc9/0x200
[   20.042194]  ? __x64_sys_io_uring_enter+0x140/0x660
[   20.042583]  __x64_sys_io_uring_enter+0x47f/0x660
[   20.042949]  ? io_sq_thread+0x4f0/0x4f0
[   20.043250]  ? trace_hardirqs_on_thunk+0x1a/0x20
[   20.043605]  ? mark_held_locks+0x24/0x90
[   20.043927]  ? do_syscall_64+0x14/0x260
[   20.044231]  do_syscall_64+0x62/0x260
[   20.044638]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   20.045182] RIP: 0033:0x7f5f0bda5e9d
[   20.045425] 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 b3 5f 0c 00 f7 d8 64 89 01 48

-- 
Jens Axboe

Re: [PATCH] io_uring: Fix leaking double_put()

[Pavel Begunkov]

[-- Attachment #1.1: Type: text/plain, Size: 745 bytes --]

On 12/11/2019 18:02, Jens Axboe wrote:
> On 11/12/19 12:17 AM, Pavel Begunkov wrote:
>> io_double_put_req() may be called for a request with a link (see
>> io_req_defer(req)), and so can leak it in case of an error, as
>> __io_free_req() doesn't handle links.
>>
>> Fixes: 78e19bbef38362ceb ("io_uring: pass in io_kiocb to fill/add CQ
>> handlers")
> 
> This blows up the 'link' test from the liburing regression suite:

> -			io_double_put_req(link);
> +			if (refcount_sub_and_test(2, &req->refs))
> +				__io_free_req(req);

My bad, it frees @req instead of @link. Sorry for the trouble.

It wouldn't apply properly anyway, as there is new commit using
io_double_put_req(). I'll resend it later.


-- 
Pavel Begunkov


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]