yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone
9ae3a8
From 4283e6a300b8b05f8fb32dc27dfe0bd774ef16db Mon Sep 17 00:00:00 2001
9ae3a8
From: Fam Zheng <famz@redhat.com>
9ae3a8
Date: Fri, 30 May 2014 03:46:55 +0200
9ae3a8
Subject: [PATCH 08/13] aio: Fix use-after-free in cancellation path
9ae3a8
9ae3a8
RH-Author: Fam Zheng <famz@redhat.com>
9ae3a8
Message-id: <1401421615-17300-1-git-send-email-famz@redhat.com>
9ae3a8
Patchwork-id: 59078
9ae3a8
O-Subject: [RHEL-7 qemu-kvm PATCH] aio: Fix use-after-free in cancellation path
9ae3a8
Bugzilla: 1095877
9ae3a8
RH-Acked-by: Paolo Bonzini <pbonzini@redhat.com>
9ae3a8
RH-Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
9ae3a8
RH-Acked-by: Kevin Wolf <kwolf@redhat.com>
9ae3a8
9ae3a8
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1095877
9ae3a8
Brew:     https://brewweb.devel.redhat.com/taskinfo?taskID=7519118 (RHEL)
9ae3a8
          https://brewweb.devel.redhat.com/taskinfo?taskID=7519129 (RHEV)
9ae3a8
9ae3a8
The current flow of canceling a thread from THREAD_ACTIVE state is:
9ae3a8
9ae3a8
  1) Caller wants to cancel a request, so it calls thread_pool_cancel.
9ae3a8
9ae3a8
  2) thread_pool_cancel waits on the conditional variable
9ae3a8
     elem->check_cancel.
9ae3a8
9ae3a8
  3) The worker thread changes state to THREAD_DONE once the task is
9ae3a8
     done, and notifies elem->check_cancel to allow thread_pool_cancel
9ae3a8
     to continue execution, and signals the notifier (pool->notifier) to
9ae3a8
     allow callback function to be called later. But because of the
9ae3a8
     global mutex, the notifier won't get processed until step 4) and 5)
9ae3a8
     are done.
9ae3a8
9ae3a8
  4) thread_pool_cancel continues, leaving the notifier signaled, it
9ae3a8
     just returns to caller.
9ae3a8
9ae3a8
  5) Caller thinks the request is already canceled successfully, so it
9ae3a8
     releases any related data, such as freeing elem->common.opaque.
9ae3a8
9ae3a8
  6) In the next main loop iteration, the notifier handler,
9ae3a8
     event_notifier_ready, is called. It finds the canceled thread in
9ae3a8
     THREAD_DONE state, so calls elem->common.cb, with an (likely)
9ae3a8
     dangling opaque pointer. This is a use-after-free.
9ae3a8
9ae3a8
Fix it by calling event_notifier_ready before leaving
9ae3a8
thread_pool_cancel.
9ae3a8
9ae3a8
Test case update: This change will let cancel complete earlier than
9ae3a8
test-thread-pool.c expects, so update the code to check this case: if
9ae3a8
it's already done, done_cb sets .aiocb to NULL, skip calling
9ae3a8
bdrv_aio_cancel on them.
9ae3a8
9ae3a8
Reported-by: Ulrich Obergfell <uobergfe@redhat.com>
9ae3a8
Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
9ae3a8
Signed-off-by: Fam Zheng <famz@redhat.com>
9ae3a8
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
9ae3a8
(cherry picked from commit 271c0f68b4eae72691721243a1c37f46a3232d61)
9ae3a8
Signed-off-by: Fam Zheng <famz@redhat.com>
9ae3a8
---
9ae3a8
 tests/test-thread-pool.c | 2 +-
9ae3a8
 thread-pool.c            | 1 +
9ae3a8
 2 files changed, 2 insertions(+), 1 deletion(-)
9ae3a8
9ae3a8
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
9ae3a8
---
9ae3a8
 tests/test-thread-pool.c |    2 +-
9ae3a8
 thread-pool.c            |    1 +
9ae3a8
 2 files changed, 2 insertions(+), 1 deletions(-)
9ae3a8
9ae3a8
diff --git a/tests/test-thread-pool.c b/tests/test-thread-pool.c
9ae3a8
index b62338f..1be970d 100644
9ae3a8
--- a/tests/test-thread-pool.c
9ae3a8
+++ b/tests/test-thread-pool.c
9ae3a8
@@ -181,7 +181,7 @@ static void test_cancel(void)
9ae3a8
 
9ae3a8
     /* Canceling the others will be a blocking operation.  */
9ae3a8
     for (i = 0; i < 100; i++) {
9ae3a8
-        if (data[i].n != 3) {
9ae3a8
+        if (data[i].aiocb && data[i].n != 3) {
9ae3a8
             bdrv_aio_cancel(data[i].aiocb);
9ae3a8
         }
9ae3a8
     }
9ae3a8
diff --git a/thread-pool.c b/thread-pool.c
9ae3a8
index 0ebd4c2..fc6a33b 100644
9ae3a8
--- a/thread-pool.c
9ae3a8
+++ b/thread-pool.c
9ae3a8
@@ -229,6 +229,7 @@ static void thread_pool_cancel(BlockDriverAIOCB *acb)
9ae3a8
         pool->pending_cancellations--;
9ae3a8
     }
9ae3a8
     qemu_mutex_unlock(&pool->lock);
9ae3a8
+    event_notifier_ready(&pool->notifier);
9ae3a8
 }
9ae3a8
 
9ae3a8
 static const AIOCBInfo thread_pool_aiocb_info = {
9ae3a8
-- 
9ae3a8
1.7.1
9ae3a8