librbd: fix use-after-free releasing object map locks in deep copy

DeepCopyRequest::send_copy_object_map() holds the destination image's
owner_lock and image_lock (shared), issues an asynchronous
object_map->rollback(), and then releases the two locks through
m_dst_image_ctx.  The rollback callback drives the rest of the request to
completion (handle_copy_object_map() ... finish() -> put()) on other
threads.  That path needs image_lock, so it only proceeds once this method
releases it, and can then run to completion and free 'this' before the
following owner_lock.unlock_shared() executes, dereferencing the freed
DeepCopyRequest.

ASan reported this as a heap-use-after-free in
TestImageReplayer.StartReplayAndWrite: the object was freed on the finisher
thread (handle_copy_metadata() -> finish()) while the io_context thread was
still in send_copy_object_map().

The destination image ctx outlives the request, so operate through a local
reference to it and release the locks through that reference rather than
through 'this', as
operation/SnapshotRollbackRequest::send_rollback_object_map() already does.

Keep the explicit lock_shared()/unlock_shared() calls rather than scoped
std::shared_lock guards: unlike
operation/SnapshotRollbackRequest::send_rollback_object_map(),
send_copy_object_map() has four exit paths (send_copy_metadata(),
send_refresh_object_map(), finish(), and the object map rollback), each of
which must release the locks before running a different continuation, so
RAII guards would push that continuation out of the locked scope and read
less clearly than unlocking at each exit.

Fixes: https://tracker.ceph.com/issues/77551
Signed-off-by: Kefu Chai <k.chai@proxmox.com>

diff --git a/src/librbd/DeepCopyRequest.cc b/src/librbd/DeepCopyRequest.cc
index 6878f145f26756add98c0db654807945d3960d70..934dad4a4ba7cb8675ecc8ec12a38b507bb61289 100644 (file)
--- a/src/librbd/DeepCopyRequest.cc
+++ b/src/librbd/DeepCopyRequest.cc
@@ -197,36 +197,38 @@ void DeepCopyRequest<I>::handle_copy_image(int r) {
 
 template <typename I>
 void DeepCopyRequest<I>::send_copy_object_map() {
-  m_dst_image_ctx->owner_lock.lock_shared();
-  m_dst_image_ctx->image_lock.lock_shared();
-
-  if (!m_dst_image_ctx->test_features(RBD_FEATURE_OBJECT_MAP,
-                                      m_dst_image_ctx->image_lock)) {
-    m_dst_image_ctx->image_lock.unlock_shared();
-    m_dst_image_ctx->owner_lock.unlock_shared();
+  // local ref so the unlocks don't touch 'this', which rollback() may free
+  I &dst_image_ctx = *m_dst_image_ctx;
+  dst_image_ctx.owner_lock.lock_shared();
+  dst_image_ctx.image_lock.lock_shared();
+
+  if (!dst_image_ctx.test_features(RBD_FEATURE_OBJECT_MAP,
+                                   dst_image_ctx.image_lock)) {
+    dst_image_ctx.image_lock.unlock_shared();
+    dst_image_ctx.owner_lock.unlock_shared();
     send_copy_metadata();
     return;
   }
   if (m_src_snap_id_end == CEPH_NOSNAP) {
-    m_dst_image_ctx->image_lock.unlock_shared();
-    m_dst_image_ctx->owner_lock.unlock_shared();
+    dst_image_ctx.image_lock.unlock_shared();
+    dst_image_ctx.owner_lock.unlock_shared();
     send_refresh_object_map();
     return;
   }
 
-  ceph_assert(m_dst_image_ctx->object_map != nullptr);
+  ceph_assert(dst_image_ctx.object_map != nullptr);
 
   ldout(m_cct, 20) << dendl;
 
   Context *finish_op_ctx = nullptr;
   int r;
-  if (m_dst_image_ctx->exclusive_lock != nullptr) {
-    finish_op_ctx = m_dst_image_ctx->exclusive_lock->start_op(&r);
+  if (dst_image_ctx.exclusive_lock != nullptr) {
+    finish_op_ctx = dst_image_ctx.exclusive_lock->start_op(&r);
   }
   if (finish_op_ctx == nullptr) {
     lderr(m_cct) << "lost exclusive lock" << dendl;
-    m_dst_image_ctx->image_lock.unlock_shared();
-    m_dst_image_ctx->owner_lock.unlock_shared();
+    dst_image_ctx.image_lock.unlock_shared();
+    dst_image_ctx.owner_lock.unlock_shared();
     finish(r);
     return;
   }
@@ -238,9 +240,9 @@ void DeepCopyRequest<I>::send_copy_object_map() {
     });
   ceph_assert(m_snap_seqs->count(m_src_snap_id_end) > 0);
   librados::snap_t copy_snap_id = (*m_snap_seqs)[m_src_snap_id_end];
-  m_dst_image_ctx->object_map->rollback(copy_snap_id, ctx);
-  m_dst_image_ctx->image_lock.unlock_shared();
-  m_dst_image_ctx->owner_lock.unlock_shared();
+  dst_image_ctx.object_map->rollback(copy_snap_id, ctx);
+  dst_image_ctx.image_lock.unlock_shared();
+  dst_image_ctx.owner_lock.unlock_shared();
 }
 
 template <typename I>