librbd: properly hold snap/parent locks during IO

The ImageCtx::parent pointer was dereferenced without holding the lock
which could lead to a crash. The ImageCtx::migration_info structure
was also dereferenced without holding a lock.

Signed-off-by: Jason Dillaman <dillaman@redhat.com>

diff --git a/src/librbd/io/CopyupRequest.cc b/src/librbd/io/CopyupRequest.cc
index 334f0ec41623fba2687d7a70e5a7eb1c24b03bc8..dc764a1d7fbbc405089d3688f84e0eaf1d0827b6 100644 (file)
--- a/src/librbd/io/CopyupRequest.cc
+++ b/src/librbd/io/CopyupRequest.cc
@@ -6,7 +6,7 @@
 #include "common/dout.h"
 #include "common/errno.h"
 #include "common/Mutex.h"
-
+#include "common/WorkQueue.h"
 #include "librbd/AsyncObjectThrottle.h"
 #include "librbd/ExclusiveLock.h"
 #include "librbd/ImageCtx.h"
@@ -101,12 +101,15 @@ CopyupRequest<I>::~CopyupRequest() {
 
 template <typename I>
 void CopyupRequest<I>::append_request(AbstractObjectWriteRequest<I> *req) {
+  ceph_assert(m_ictx->copyup_list_lock.is_locked());
+
   ldout(m_ictx->cct, 20) << req << dendl;
   m_pending_requests.push_back(req);
 }
 
 template <typename I>
 void CopyupRequest<I>::complete_requests(int r) {
+  // already removed from copyup list
   while (!m_pending_requests.empty()) {
     auto it = m_pending_requests.begin();
     auto req = *it;
@@ -235,6 +238,8 @@ bool CopyupRequest<I>::is_update_object_map_required(int r) {
 
 template <typename I>
 bool CopyupRequest<I>::is_deep_copy() const {
+  ceph_assert(m_ictx->snap_lock.is_locked());
+
   return !m_ictx->migration_info.empty();
 }
 
@@ -243,20 +248,40 @@ void CopyupRequest<I>::send()
 {
   m_state = STATE_READ_FROM_PARENT;
 
+  m_ictx->snap_lock.get_read();
+  m_ictx->parent_lock.get_read();
+  if (m_ictx->parent == nullptr) {
+    ldout(m_ictx->cct, 5) << "parent detached" << dendl;
+    m_ictx->parent_lock.put_read();
+    m_ictx->snap_lock.put_read();
+
+    m_ictx->op_work_queue->queue(util::create_context_callback(this), -ENOENT);
+    return;
+  }
+
   if (is_deep_copy()) {
+    m_ictx->copyup_list_lock.Lock();
     m_flatten = is_copyup_required() ? true : m_ictx->migration_info.flatten;
+    m_ictx->copyup_list_lock.Unlock();
+
+    m_deep_copy = true;
     auto req = deep_copy::ObjectCopyRequest<I>::create(
-        m_ictx->parent, m_ictx, m_ictx->migration_info.snap_map, m_object_no,
-        m_flatten, util::create_context_callback(this));
+      m_ictx->parent, m_ictx, m_ictx->migration_info.snap_map, m_object_no,
+      m_flatten, util::create_context_callback(this));
+
     ldout(m_ictx->cct, 20) << "deep copy object req " << req
                            << ", object_no " << m_object_no
                            << ", flatten " << m_flatten
                            << dendl;
     req->send();
+
+    m_ictx->parent_lock.put_read();
+    m_ictx->snap_lock.put_read();
     return;
   }
 
-  AioCompletion *comp = AioCompletion::create_and_start(
+  m_deep_copy = false;
+  auto comp = AioCompletion::create_and_start(
     this, m_ictx, AIO_TYPE_READ);
 
   ldout(m_ictx->cct, 20) << "completion " << comp
@@ -265,6 +290,9 @@ void CopyupRequest<I>::send()
                          << dendl;
   ImageRequest<>::aio_read(m_ictx->parent, comp, std::move(m_image_extents),
                            ReadResult{&m_copyup_data}, 0, m_trace);
+
+  m_ictx->parent_lock.put_read();
+  m_ictx->snap_lock.put_read();
 }
 
 template <typename I>
@@ -286,24 +314,27 @@ bool CopyupRequest<I>::should_complete(int *r) {
   switch (m_state) {
   case STATE_READ_FROM_PARENT:
     ldout(cct, 20) << "READ_FROM_PARENT" << dendl;
+
     m_ictx->copyup_list_lock.Lock();
-    if (*r == -ENOENT && is_deep_copy() && !m_flatten && is_copyup_required()) {
+    if (*r == -ENOENT && m_deep_copy && !m_flatten && is_copyup_required()) {
       ldout(cct, 5) << "restart deep copy with flatten" << dendl;
       m_ictx->copyup_list_lock.Unlock();
+
       send();
       return false;
     }
+
     remove_from_list(m_ictx->copyup_list_lock);
     m_ictx->copyup_list_lock.Unlock();
+
     if (*r >= 0 || *r == -ENOENT) {
       if (!is_copyup_required() && !is_update_object_map_required(*r)) {
-        if (*r == -ENOENT && is_deep_copy()) {
+        if (*r == -ENOENT && m_deep_copy) {
           *r = 0;
         }
         ldout(cct, 20) << "skipping" << dendl;
         return true;
       }
-
       return send_object_map_head();
     }
     break;

diff --git a/src/librbd/io/CopyupRequest.h b/src/librbd/io/CopyupRequest.h
index d091e8775293b764efaaf46aa51bc624c020f215..0926f904d6697c2dbbff3236cbec69a54c783c08 100644 (file)
--- a/src/librbd/io/CopyupRequest.h
+++ b/src/librbd/io/CopyupRequest.h
@@ -92,7 +92,8 @@ private:
   ZTracer::Trace m_trace;
 
   State m_state;
-  bool m_flatten;
+  bool m_deep_copy = false;
+  bool m_flatten = false;
   ceph::bufferlist m_copyup_data;
   std::vector<AbstractObjectWriteRequest<ImageCtxT> *> m_pending_requests;
   unsigned m_pending_copyups = 0;

diff --git a/src/librbd/io/ObjectRequest.cc b/src/librbd/io/ObjectRequest.cc
index 611e83677a6f5ab63955af2234e76418adfcd02e..61cd787c6cd1cb30507f93d91422b0e1c6d1b3e1 100644 (file)
--- a/src/librbd/io/ObjectRequest.cc
+++ b/src/librbd/io/ObjectRequest.cc
@@ -266,38 +266,42 @@ template <typename I>
 void ObjectReadRequest<I>::read_parent() {
   I *image_ctx = this->m_ictx;
 
-  uint64_t object_overlap = 0;
+  image_ctx->snap_lock.get_read();
+  image_ctx->parent_lock.get_read();
+
+  // calculate reverse mapping onto the image
   Extents parent_extents;
-  {
-    RWLock::RLocker snap_locker(image_ctx->snap_lock);
-    RWLock::RLocker parent_locker(image_ctx->parent_lock);
+  Striper::extent_to_file(image_ctx->cct, &image_ctx->layout,
+                          this->m_object_no, this->m_object_off,
+                          this->m_object_len, parent_extents);
 
-    // calculate reverse mapping onto the image
-    Striper::extent_to_file(image_ctx->cct, &image_ctx->layout,
-                            this->m_object_no, this->m_object_off,
-                            this->m_object_len, parent_extents);
-
-    uint64_t parent_overlap = 0;
-    int r = image_ctx->get_parent_overlap(this->m_snap_id, &parent_overlap);
-    if (r == 0) {
-      object_overlap = image_ctx->prune_parent_extents(parent_extents,
-                                                       parent_overlap);
-    }
+  uint64_t parent_overlap = 0;
+  uint64_t object_overlap = 0;
+  int r = image_ctx->get_parent_overlap(this->m_snap_id, &parent_overlap);
+  if (r == 0) {
+    object_overlap = image_ctx->prune_parent_extents(parent_extents,
+                                                     parent_overlap);
   }
 
   if (object_overlap == 0) {
+    image_ctx->parent_lock.put_read();
+    image_ctx->snap_lock.put_read();
+
     this->finish(-ENOENT);
     return;
   }
 
   ldout(image_ctx->cct, 20) << dendl;
 
-  AioCompletion *parent_completion = AioCompletion::create_and_start<
+  auto parent_completion = AioCompletion::create_and_start<
     ObjectReadRequest<I>, &ObjectReadRequest<I>::handle_read_parent>(
       this, util::get_image_ctx(image_ctx->parent), AIO_TYPE_READ);
   ImageRequest<I>::aio_read(image_ctx->parent, parent_completion,
                             std::move(parent_extents), ReadResult{m_read_data},
                             0, this->m_trace);
+
+  image_ctx->parent_lock.put_read();
+  image_ctx->snap_lock.put_read();
 }
 
 template <typename I>
@@ -384,6 +388,12 @@ AbstractObjectWriteRequest<I>::AbstractObjectWriteRequest(
   }
 
   compute_parent_info();
+
+  ictx->snap_lock.get_read();
+  if (!ictx->migration_info.empty()) {
+    m_guarding_migration_write = true;
+  }
+  ictx->snap_lock.put_read();
 }
 
 template <typename I>
@@ -498,8 +508,7 @@ void AbstractObjectWriteRequest<I>::write_object() {
   librados::ObjectWriteOperation write;
   if (m_copyup_enabled) {
     ldout(image_ctx->cct, 20) << "guarding write" << dendl;
-    if (!image_ctx->migration_info.empty()) {
-      m_guarding_migration_write = true;
+    if (m_guarding_migration_write) {
       cls_client::assert_snapc_seq(
         &write, m_snap_seq, cls::rbd::ASSERT_SNAPC_SEQ_LE_SNAPSET_SEQ);
     } else {
@@ -533,11 +542,14 @@ void AbstractObjectWriteRequest<I>::handle_write_object(int r) {
       return;
     }
   } else if (r == -ERANGE && m_guarding_migration_write) {
-    if (!image_ctx->migration_info.empty()) {
+    image_ctx->snap_lock.get_read();
+    m_guarding_migration_write = !image_ctx->migration_info.empty();
+    image_ctx->snap_lock.put_read();
+
+    if (m_guarding_migration_write) {
       copyup();
     } else {
       ldout(image_ctx->cct, 10) << "migration parent gone, restart io" << dendl;
-      m_guarding_migration_write = false;
       compute_parent_info();
       write_object();
     }

diff --git a/src/test/librbd/io/test_mock_ObjectRequest.cc b/src/test/librbd/io/test_mock_ObjectRequest.cc
index 98cd102629bd25a0e5f848b56cc4dde58daa8212..7cf6271907bb1f1fe4dfa724443afb6b88ebc8d8 100644 (file)
--- a/src/test/librbd/io/test_mock_ObjectRequest.cc
+++ b/src/test/librbd/io/test_mock_ObjectRequest.cc
@@ -180,13 +180,13 @@ struct TestMockIoObjectRequest : public TestMockFixture {
     }
   }
 
-  void expect_aio_read(MockImageRequest& mock_image_request,
+  void expect_aio_read(MockTestImageCtx &mock_image_ctx,
+                       MockImageRequest& mock_image_request,
                        Extents&& extents, int r) {
     EXPECT_CALL(mock_image_request, aio_read(_, extents))
-      .WillOnce(WithArg<0>(Invoke([r](AioCompletion* aio_comp) {
+      .WillOnce(WithArg<0>(Invoke([&mock_image_ctx, r](AioCompletion* aio_comp) {
                              aio_comp->set_request_count(1);
-                             aio_comp->add_request();
-                             aio_comp->complete_request(r);
+                             mock_image_ctx.image_ctx->op_work_queue->queue(new C_AioRequest(aio_comp), r);
                            })));
   }
 
@@ -430,7 +430,7 @@ TEST_F(TestMockIoObjectRequest, ParentRead) {
   MockImageRequest mock_image_request;
   expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 4096, 0);
   expect_prune_parent_extents(mock_image_ctx, {{0, 4096}}, 4096, 4096);
-  expect_aio_read(mock_image_request, {{0, 4096}}, 0);
+  expect_aio_read(mock_image_ctx, mock_image_request, {{0, 4096}}, 0);
 
   bufferlist bl;
   ExtentMap extent_map;
@@ -478,7 +478,7 @@ TEST_F(TestMockIoObjectRequest, ParentReadError) {
   MockImageRequest mock_image_request;
   expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 4096, 0);
   expect_prune_parent_extents(mock_image_ctx, {{0, 4096}}, 4096, 4096);
-  expect_aio_read(mock_image_request, {{0, 4096}}, -EPERM);
+  expect_aio_read(mock_image_ctx, mock_image_request, {{0, 4096}}, -EPERM);
 
   bufferlist bl;
   ExtentMap extent_map;
@@ -526,7 +526,7 @@ TEST_F(TestMockIoObjectRequest, CopyOnRead) {
   MockImageRequest mock_image_request;
   expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 4096, 0);
   expect_prune_parent_extents(mock_image_ctx, {{0, 4096}}, 4096, 4096);
-  expect_aio_read(mock_image_request, {{0, 4096}}, 0);
+  expect_aio_read(mock_image_ctx, mock_image_request, {{0, 4096}}, 0);
 
   MockCopyupRequest mock_copyup_request;
   expect_get_parent_overlap(mock_image_ctx, CEPH_NOSNAP, 4096, 0);