librbd/cache/pwl/ssd: fix use-after-free on C_BlockIORequest

In setup_schedule_append() function, its first expression
will cause the req to be deleted, and subsequent use of
the variable req becomes an illegal operation. And due to
delete, rep->m_image_ctx will be empty, so it lead to
segfault in AbstractWriteLog::get_context().
So pass the `req` into `schedule_append()` function.

Fixes: https://tracker.ceph.com/issues/50951
Signed-off-by: Hualong Feng <hualong.feng@intel.com>

diff --git a/src/librbd/cache/pwl/AbstractWriteLog.cc b/src/librbd/cache/pwl/AbstractWriteLog.cc
index 0dbe6d5a46329ce6c2afb9d974ca37c119ac502e..20a03e6b350f69d612c8a0b56339cf058e52605f 100644 (file)
--- a/src/librbd/cache/pwl/AbstractWriteLog.cc
+++ b/src/librbd/cache/pwl/AbstractWriteLog.cc
@@ -1256,19 +1256,19 @@ void AbstractWriteLog<I>::append_scheduled(GenericLogOperations &ops, bool &ops_
 }
 
 template <typename I>
-void AbstractWriteLog<I>::schedule_append(GenericLogOperationsVector &ops)
+void AbstractWriteLog<I>::schedule_append(GenericLogOperationsVector &ops, C_BlockIORequestT *req)
 {
   GenericLogOperations to_append(ops.begin(), ops.end());
 
-  schedule_append_ops(to_append);
+  schedule_append_ops(to_append, req);
 }
 
 template <typename I>
-void AbstractWriteLog<I>::schedule_append(GenericLogOperationSharedPtr op)
+void AbstractWriteLog<I>::schedule_append(GenericLogOperationSharedPtr op, C_BlockIORequestT *req)
 {
   GenericLogOperations to_append { op };
 
-  schedule_append_ops(to_append);
+  schedule_append_ops(to_append, req);
 }
 
 /*

diff --git a/src/librbd/cache/pwl/AbstractWriteLog.h b/src/librbd/cache/pwl/AbstractWriteLog.h
index b63a95ebb7e6e7461ed2445dbe8a19faa6f68c16..2ca831f1f99cfc323a7ffcc17c5edc8ae5d7ee32 100644 (file)
--- a/src/librbd/cache/pwl/AbstractWriteLog.h
+++ b/src/librbd/cache/pwl/AbstractWriteLog.h
@@ -125,8 +125,8 @@ public:
   virtual void setup_schedule_append(
       pwl::GenericLogOperationsVector &ops, bool do_early_flush,
       C_BlockIORequestT *req) = 0;
-  void schedule_append(pwl::GenericLogOperationsVector &ops);
-  void schedule_append(pwl::GenericLogOperationSharedPtr op);
+  void schedule_append(pwl::GenericLogOperationsVector &ops, C_BlockIORequestT *req = nullptr);
+  void schedule_append(pwl::GenericLogOperationSharedPtr op, C_BlockIORequestT *req = nullptr);
   void flush_new_sync_point(C_FlushRequestT *flush_req,
                             pwl::DeferredContexts &later);
 
@@ -360,7 +360,7 @@ protected:
 
   virtual void process_work() = 0;
   virtual void append_scheduled_ops(void) = 0;
-  virtual void schedule_append_ops(pwl::GenericLogOperations &ops) = 0;
+  virtual void schedule_append_ops(pwl::GenericLogOperations &ops, C_BlockIORequestT *req) = 0;
   virtual void remove_pool_file() = 0;
   virtual bool initialize_pool(Context *on_finish,
                                pwl::DeferredContexts &later) = 0;

diff --git a/src/librbd/cache/pwl/rwl/WriteLog.cc b/src/librbd/cache/pwl/rwl/WriteLog.cc
index 5530d900c4bcf877402ae7010efbb5f4f88ef164..ef0735ba65b903dbd8139852a31a7b79a514491e 100644 (file)
--- a/src/librbd/cache/pwl/rwl/WriteLog.cc
+++ b/src/librbd/cache/pwl/rwl/WriteLog.cc
@@ -625,7 +625,7 @@ void WriteLog<I>::flush_then_append_scheduled_ops(void)
      * get to the log message append step. */
     if (ops.size()) {
       flush_pmem_buffer(ops);
-      schedule_append_ops(ops);
+      schedule_append_ops(ops, nullptr);
     }
   } while (ops_remain);
   append_scheduled_ops();
@@ -695,7 +695,7 @@ void WriteLog<I>::setup_schedule_append(
  * all prior log entries are persisted everywhere.
  */
 template <typename I>
-void WriteLog<I>::schedule_append_ops(GenericLogOperations &ops)
+void WriteLog<I>::schedule_append_ops(GenericLogOperations &ops, C_BlockIORequestT *req)
 {
   bool need_finisher;
   GenericLogOperationsVector appending;

diff --git a/src/librbd/cache/pwl/rwl/WriteLog.h b/src/librbd/cache/pwl/rwl/WriteLog.h
index 17b0d4388dcdb085e94eb07dc83f7b19d7422a24..92989ac21a5d63cdb5248858a1636fed530caac9 100644 (file)
--- a/src/librbd/cache/pwl/rwl/WriteLog.h
+++ b/src/librbd/cache/pwl/rwl/WriteLog.h
@@ -82,7 +82,7 @@ protected:
   using AbstractWriteLog<ImageCtxT>::m_first_valid_entry;
 
   void process_work() override;
-  void schedule_append_ops(pwl::GenericLogOperations &ops) override;
+  void schedule_append_ops(pwl::GenericLogOperations &ops, C_BlockIORequestT *req) override;
   void append_scheduled_ops(void) override;
   void reserve_cache(C_BlockIORequestT *req,
                      bool &alloc_succeeds, bool &no_space) override;

diff --git a/src/librbd/cache/pwl/ssd/WriteLog.cc b/src/librbd/cache/pwl/ssd/WriteLog.cc
index b85e45544ee00aeee217f44745df79a64d409965..4d3bae39782a4b1c10256fc0118b2005121b9e8e 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.cc
+++ b/src/librbd/cache/pwl/ssd/WriteLog.cc
@@ -316,7 +316,7 @@ void WriteLog<I>::enlist_op_appender() {
  * all prior log entries are persisted everywhere.
  */
 template<typename I>
-void WriteLog<I>::schedule_append_ops(GenericLogOperations &ops) {
+void WriteLog<I>::schedule_append_ops(GenericLogOperations &ops, C_BlockIORequestT *req) {
   bool need_finisher = false;
   GenericLogOperationsVector appending;
 
@@ -334,6 +334,17 @@ void WriteLog<I>::schedule_append_ops(GenericLogOperations &ops) {
       need_finisher = has_sync_point_logs(ops);
     }
     this->m_ops_to_append.splice(this->m_ops_to_append.end(), ops);
+
+    // To preserve the order of overlapping IOs, release_cell() may be
+    // called only after the ops are added to m_ops_to_append.
+    // As soon as m_lock is released, the appended ops can be picked up
+    // by append_scheduled_ops() in another thread and req can be freed.
+    if (req != nullptr) {
+      if (persist_on_flush) {
+        req->complete_user_request(0);
+      }
+      req->release_cell();
+    }
   }
 
   if (need_finisher) {
@@ -349,11 +360,7 @@ template <typename I>
 void WriteLog<I>::setup_schedule_append(pwl::GenericLogOperationsVector &ops,
                                         bool do_early_flush,
                                         C_BlockIORequestT *req) {
-  this->schedule_append(ops);
-  if (this->get_persist_on_flush()) {
-    req->complete_user_request(0);
-  }
-  req->release_cell();
+  this->schedule_append(ops, req);
 }
 
 template <typename I>

diff --git a/src/librbd/cache/pwl/ssd/WriteLog.h b/src/librbd/cache/pwl/ssd/WriteLog.h
index 1a7a5e7a11629228f859a574820cd06c4153fbb7..f71b6d5ab11da12497c45228f35ae8a3c939b7ab 100644 (file)
--- a/src/librbd/cache/pwl/ssd/WriteLog.h
+++ b/src/librbd/cache/pwl/ssd/WriteLog.h
@@ -67,7 +67,7 @@ protected:
                        pwl::DeferredContexts &later) override;
   void process_work() override;
   void append_scheduled_ops(void) override;
-  void schedule_append_ops(pwl::GenericLogOperations &ops) override;
+  void schedule_append_ops(pwl::GenericLogOperations &ops, C_BlockIORequestT *req) override;
   void remove_pool_file() override;
   void release_ram(std::shared_ptr<GenericLogEntry> log_entry) override;