* [PATCH] io_uring: Fix leaking double_put()
@ 2019-11-12 8:17 Pavel Begunkov
2019-11-12 15:02 ` Jens Axboe
0 siblings, 1 reply; 3+ messages in thread
From: Pavel Begunkov @ 2019-11-12 8:17 UTC (permalink / raw)
To: Jens Axboe, io-uring
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] io_uring: Fix leaking double_put()
2019-11-12 8:17 [PATCH] io_uring: Fix leaking double_put() Pavel Begunkov
@ 2019-11-12 15:02 ` Jens Axboe
2019-11-12 20:11 ` Pavel Begunkov
0 siblings, 1 reply; 3+ messages in thread
From: Jens Axboe @ 2019-11-12 15:02 UTC (permalink / raw)
To: Pavel Begunkov, io-uring
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] io_uring: Fix leaking double_put()
2019-11-12 15:02 ` Jens Axboe
@ 2019-11-12 20:11 ` Pavel Begunkov
0 siblings, 0 replies; 3+ messages in thread
From: Pavel Begunkov @ 2019-11-12 20:11 UTC (permalink / raw)
To: Jens Axboe, io-uring
[-- 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 --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-12 20:12 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-12 8:17 [PATCH] io_uring: Fix leaking double_put() Pavel Begunkov
2019-11-12 15:02 ` Jens Axboe
2019-11-12 20:11 ` Pavel Begunkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).