The current flow of canceling a thread from THREAD_ACTIVE state is:
1) Caller wants to cancel a request, so it calls thread_pool_cancel.
2) thread_pool_cancel waits on the conditional variable
elem->check_cancel.
3) The worker thread changes state to THREAD_DONE once the task is
done, and notifies elem->check_cancel to allow thread_pool_cancel
to continue execution, and signals the notifier (pool->notifier) to
allow callback function to be called later. But because of the
global mutex, the notifier won't get processed until step 4) and 5)
are done.
4) thread_pool_cancel continues, leaving the notifier signaled, it
just returns to caller.
5) Caller thinks the request is already canceled successfully, so it
releases any related data, such as freeing elem->common.opaque.
6) In the next main loop iteration, the notifier handler,
event_notifier_ready, is called. It finds the canceled thread in
THREAD_DONE state, so calls elem->common.cb, with an (likely)
dangling opaque pointer. This is a use-after-free.
Fix it by calling elem->common.cb right before thread_pool_cancel
returns.
We leave the notifier signaled to allow any other acbs to be processed.
Reported-by: Ulrich Obergfell <uober...@redhat.com>
Suggested-by: Stefan Hajnoczi <stefa...@redhat.com>
Signed-off-by: Fam Zheng <f...@redhat.com>
---
thread-pool.c | 40 ++++++++++++++++++++++++++++++----------
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/thread-pool.c b/thread-pool.c
index fbdd3ff..cc1c9f1 100644
--- a/thread-pool.c
+++ b/thread-pool.c
@@ -168,6 +168,24 @@ static void spawn_thread(ThreadPool *pool)
}
}
+/* Complete one thread and call common.cb if it has returned.
+ * Returns true if commmon.cb is called.
+ */
+static bool thread_pool_complete_one(ThreadPoolElement *elem)
+{
+ bool ret = false;
+
+ QLIST_REMOVE(elem, all);
+ if (elem->state == THREAD_DONE && elem->common.cb) {
+ /* Read state before ret. */
+ smp_rmb();
+ elem->common.cb(elem->common.opaque, elem->ret);
+ ret = true;
+ }
+ qemu_aio_release(elem);
+ return ret;
+}
+
static void event_notifier_ready(EventNotifier *notifier)
{
ThreadPool *pool = container_of(notifier, ThreadPool, notifier);
@@ -183,23 +201,15 @@ restart:
trace_thread_pool_complete(pool, elem, elem->common.opaque,
elem->ret);
}
- if (elem->state == THREAD_DONE && elem->common.cb) {
- QLIST_REMOVE(elem, all);
- /* Read state before ret. */
- smp_rmb();
- elem->common.cb(elem->common.opaque, elem->ret);
- qemu_aio_release(elem);
+ if (thread_pool_complete_one(elem)) {
goto restart;
- } else {
- /* remove the request */
- QLIST_REMOVE(elem, all);
- qemu_aio_release(elem);
}
}
}
static void thread_pool_cancel(BlockDriverAIOCB *acb)
{
+ ThreadPoolElement *next;
ThreadPoolElement *elem = (ThreadPoolElement *)acb;
ThreadPool *pool = elem->pool;
@@ -222,6 +232,16 @@ static void thread_pool_cancel(BlockDriverAIOCB *acb)
qemu_cond_wait(&pool->check_cancel, &pool->lock);
}
pool->pending_cancellations--;
+
+ /* In the case of THREAD_ACTIVE -> THREAD_DONE, we need to call
+ * callback. Make sure we do it before returning to caller.
+ */
+ QLIST_FOREACH_SAFE(elem, &pool->head, all, next) {
+ if (acb == &elem->common) {
+ thread_pool_complete_one(elem);
+ break;
+ }
+ }
}
qemu_mutex_unlock(&pool->lock);
}
--
1.9.2
