From 5d6134fb359f752eca7f6a5213a52a565255e89b Mon Sep 17 00:00:00 2001
From: Jason Dillaman <dillaman@redhat.com>
Date: Tue, 25 Jun 2019 10:15:25 -0400
Subject: [PATCH] librbd: prevent concurrent AIO callbacks to external clients

With the various threads and conditional IO paths within librbd, it was
possible for external AIO callbacks to be concurrently executed from
different librbd threads. These callbacks should be serialized to reduce
the unexpected potential for data corruption.

Fixes: http://tracker.ceph.com/issues/40417
Signed-off-by: Jason Dillaman <dillaman@redhat.com>
---
 src/librbd/ImageCtx.cc         |  3 ++-
 src/librbd/ImageCtx.h          |  9 +++++++--
 src/librbd/internal.cc         |  2 +-
 src/librbd/io/AioCompletion.cc | 33 +++++++++++++++++++++++++++++++--
 src/librbd/io/AioCompletion.h  |  2 ++
 src/librbd/librbd.cc           |  6 ++++--
 6 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/src/librbd/ImageCtx.cc b/src/librbd/ImageCtx.cc
index 5a8bf337745..8c5a07eacaf 100644
--- a/src/librbd/ImageCtx.cc
+++ b/src/librbd/ImageCtx.cc
@@ -122,7 +122,8 @@ public:
       operations(new Operations<>(*this)),
       exclusive_lock(nullptr), object_map(nullptr),
       io_work_queue(nullptr), op_work_queue(nullptr),
-      completed_reqs(32),
+      external_callback_completions(32),
+      event_socket_completions(32),
       asok_hook(nullptr),
       trace_endpoint("librbd")
   {
diff --git a/src/librbd/ImageCtx.h b/src/librbd/ImageCtx.h
index 32f5f5f2b96..2ae215fcab1 100644
--- a/src/librbd/ImageCtx.h
+++ b/src/librbd/ImageCtx.h
@@ -172,9 +172,14 @@ namespace librbd {
 
     ContextWQ *op_work_queue;
 
-    boost::lockfree::queue<
+    typedef boost::lockfree::queue<
       io::AioCompletion*,
-      boost::lockfree::allocator<ceph::allocator<void>>> completed_reqs;
+      boost::lockfree::allocator<ceph::allocator<void>>> Completions;
+
+    Completions external_callback_completions;
+    std::atomic<bool> external_callback_in_progress = {false};
+
+    Completions event_socket_completions;
     EventSocket event_socket;
 
     bool ignore_migrating = false;
diff --git a/src/librbd/internal.cc b/src/librbd/internal.cc
index dadf699c29d..1cefd073b52 100644
--- a/src/librbd/internal.cc
+++ b/src/librbd/internal.cc
@@ -1839,7 +1839,7 @@ int validate_pool(IoCtx &io_ctx, CephContext *cct) {
     ldout(cct, 20) << __func__ << " " << ictx << " numcomp = " << numcomp
                    << dendl;
     int i = 0;
-    while (i < numcomp && ictx->completed_reqs.pop(comps[i])) {
+    while (i < numcomp && ictx->event_socket_completions.pop(comps[i])) {
       ++i;
     }
 
diff --git a/src/librbd/io/AioCompletion.cc b/src/librbd/io/AioCompletion.cc
index 011b005dfd4..44ad7b72fdb 100644
--- a/src/librbd/io/AioCompletion.cc
+++ b/src/librbd/io/AioCompletion.cc
@@ -99,11 +99,15 @@ void AioCompletion::complete() {
 
   state = AIO_STATE_CALLBACK;
   if (complete_cb) {
-    complete_cb(rbd_comp, complete_arg);
+    if (external_callback) {
+      complete_external_callback();
+    } else {
+      complete_cb(rbd_comp, complete_arg);
+    }
   }
 
   if (ictx != nullptr && event_notify && ictx->event_socket.is_valid()) {
-    ictx->completed_reqs.push(this);
+    ictx->event_socket_completions.push(this);
     ictx->event_socket.notify();
   }
   state = AIO_STATE_COMPLETE;
@@ -242,5 +246,30 @@ ssize_t AioCompletion::get_return_value() {
   return r;
 }
 
+void AioCompletion::complete_external_callback() {
+  // ensure librbd external users never experience concurrent callbacks
+  // from multiple librbd-internal threads.
+  ictx->external_callback_completions.push(this);
+
+  while (true) {
+    if (ictx->external_callback_in_progress.exchange(true)) {
+      // another thread is concurrently invoking external callbacks
+      break;
+    }
+
+    AioCompletion* aio_comp;
+    while (ictx->external_callback_completions.pop(aio_comp)) {
+      aio_comp->complete_cb(aio_comp->rbd_comp, aio_comp->complete_arg);
+    }
+
+    ictx->external_callback_in_progress.store(false);
+    if (ictx->external_callback_completions.empty()) {
+      // queue still empty implies we didn't have a race between the last failed
+      // pop and resetting the in-progress state
+      break;
+    }
+  }
+}
+
 } // namespace io
 } // namespace librbd
diff --git a/src/librbd/io/AioCompletion.h b/src/librbd/io/AioCompletion.h
index 551596a0de8..19e978edbbc 100644
--- a/src/librbd/io/AioCompletion.h
+++ b/src/librbd/io/AioCompletion.h
@@ -70,6 +70,7 @@ struct AioCompletion {
 
   bool event_notify = false;
   bool was_armed = false;
+  bool external_callback = false;
 
   template <typename T, void (T::*MF)(int)>
   static void callback_adapter(completion_t cb, void *arg) {
@@ -176,6 +177,7 @@ struct AioCompletion {
 
 private:
   void queue_complete();
+  void complete_external_callback();
 
 };
 
diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc
index b12404fcaaa..47f5cd5a7cc 100644
--- a/src/librbd/librbd.cc
+++ b/src/librbd/librbd.cc
@@ -1155,8 +1155,10 @@ namespace librbd {
 
   RBD::AioCompletion::AioCompletion(void *cb_arg, callback_t complete_cb)
   {
-    pc = reinterpret_cast<void*>(librbd::io::AioCompletion::create(
-      cb_arg, complete_cb, this));
+    auto aio_comp = librbd::io::AioCompletion::create(
+      cb_arg, complete_cb, this);
+    aio_comp->external_callback = true;
+    pc = reinterpret_cast<void*>(aio_comp);
   }
 
   bool RBD::AioCompletion::is_complete()
-- 
2.39.5