]>
Commit | Line | Data |
---|---|---|
0ba741a2 SP |
1 | From 271c0f68b4eae72691721243a1c37f46a3232d61 Mon Sep 17 00:00:00 2001 |
2 | From: Fam Zheng <famz@redhat.com> | |
3 | Date: Wed, 21 May 2014 10:42:13 +0800 | |
4 | Subject: [PATCH] aio: Fix use-after-free in cancellation path | |
5 | ||
6 | The current flow of canceling a thread from THREAD_ACTIVE state is: | |
7 | ||
8 | 1) Caller wants to cancel a request, so it calls thread_pool_cancel. | |
9 | ||
10 | 2) thread_pool_cancel waits on the conditional variable | |
11 | elem->check_cancel. | |
12 | ||
13 | 3) The worker thread changes state to THREAD_DONE once the task is | |
14 | done, and notifies elem->check_cancel to allow thread_pool_cancel | |
15 | to continue execution, and signals the notifier (pool->notifier) to | |
16 | allow callback function to be called later. But because of the | |
17 | global mutex, the notifier won't get processed until step 4) and 5) | |
18 | are done. | |
19 | ||
20 | 4) thread_pool_cancel continues, leaving the notifier signaled, it | |
21 | just returns to caller. | |
22 | ||
23 | 5) Caller thinks the request is already canceled successfully, so it | |
24 | releases any related data, such as freeing elem->common.opaque. | |
25 | ||
26 | 6) In the next main loop iteration, the notifier handler, | |
27 | event_notifier_ready, is called. It finds the canceled thread in | |
28 | THREAD_DONE state, so calls elem->common.cb, with an (likely) | |
29 | dangling opaque pointer. This is a use-after-free. | |
30 | ||
31 | Fix it by calling event_notifier_ready before leaving | |
32 | thread_pool_cancel. | |
33 | ||
34 | Test case update: This change will let cancel complete earlier than | |
35 | test-thread-pool.c expects, so update the code to check this case: if | |
36 | it's already done, done_cb sets .aiocb to NULL, skip calling | |
37 | bdrv_aio_cancel on them. | |
38 | ||
39 | Reported-by: Ulrich Obergfell <uobergfe@redhat.com> | |
40 | Suggested-by: Paolo Bonzini <pbonzini@redhat.com> | |
41 | Signed-off-by: Fam Zheng <famz@redhat.com> | |
42 | Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> | |
43 | --- | |
44 | tests/test-thread-pool.c | 2 +- | |
45 | thread-pool.c | 1 + | |
46 | 2 files changed, 2 insertions(+), 1 deletion(-) | |
47 | ||
48 | diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c | |
49 | index c1f8e13..aa156bc 100644 | |
50 | --- a/tests/test-thread-pool.c | |
51 | +++ b/tests/test-thread-pool.c | |
52 | @@ -180,7 +180,7 @@ static void test_cancel(void) | |
53 | ||
54 | /* Canceling the others will be a blocking operation. */ | |
55 | for (i = 0; i < 100; i++) { | |
56 | - if (data[i].n != 3) { | |
57 | + if (data[i].aiocb && data[i].n != 3) { | |
58 | bdrv_aio_cancel(data[i].aiocb); | |
59 | } | |
60 | } | |
61 | diff --git a/thread-pool.c b/thread-pool.c | |
62 | index fbdd3ff..dfb699d 100644 | |
63 | --- a/thread-pool.c | |
64 | +++ b/thread-pool.c | |
65 | @@ -224,6 +224,7 @@ static void thread_pool_cancel(BlockDriverAIOCB *acb) | |
66 | pool->pending_cancellations--; | |
67 | } | |
68 | qemu_mutex_unlock(&pool->lock); | |
69 | + event_notifier_ready(&pool->notifier); | |
70 | } | |
71 | ||
72 | static const AIOCBInfo thread_pool_aiocb_info = { | |
73 | -- | |
74 | 1.7.10.4 | |
75 |