From 2dc3b8881290f1e12c536a232bea37547a73a45b Mon Sep 17 00:00:00 2001 From: Hualong Feng Date: Fri, 2 Jul 2021 09:49:39 +0800 Subject: [PATCH] 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 --- src/librbd/cache/pwl/AbstractWriteLog.cc | 8 ++++---- src/librbd/cache/pwl/AbstractWriteLog.h | 6 +++--- src/librbd/cache/pwl/rwl/WriteLog.cc | 4 ++-- src/librbd/cache/pwl/rwl/WriteLog.h | 2 +- src/librbd/cache/pwl/ssd/WriteLog.cc | 19 +++++++++++++------ src/librbd/cache/pwl/ssd/WriteLog.h | 2 +- 6 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/librbd/cache/pwl/AbstractWriteLog.cc b/src/librbd/cache/pwl/AbstractWriteLog.cc index 0dbe6d5a46329..20a03e6b350f6 100644 --- a/src/librbd/cache/pwl/AbstractWriteLog.cc +++ b/src/librbd/cache/pwl/AbstractWriteLog.cc @@ -1256,19 +1256,19 @@ void AbstractWriteLog::append_scheduled(GenericLogOperations &ops, bool &ops_ } template -void AbstractWriteLog::schedule_append(GenericLogOperationsVector &ops) +void AbstractWriteLog::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 -void AbstractWriteLog::schedule_append(GenericLogOperationSharedPtr op) +void AbstractWriteLog::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 b63a95ebb7e6e..2ca831f1f99cf 100644 --- 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 5530d900c4bcf..ef0735ba65b90 100644 --- a/src/librbd/cache/pwl/rwl/WriteLog.cc +++ b/src/librbd/cache/pwl/rwl/WriteLog.cc @@ -625,7 +625,7 @@ void WriteLog::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::setup_schedule_append( * all prior log entries are persisted everywhere. */ template -void WriteLog::schedule_append_ops(GenericLogOperations &ops) +void WriteLog::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 17b0d4388dcdb..92989ac21a5d6 100644 --- a/src/librbd/cache/pwl/rwl/WriteLog.h +++ b/src/librbd/cache/pwl/rwl/WriteLog.h @@ -82,7 +82,7 @@ protected: using AbstractWriteLog::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 b85e45544ee00..4d3bae39782a4 100644 --- a/src/librbd/cache/pwl/ssd/WriteLog.cc +++ b/src/librbd/cache/pwl/ssd/WriteLog.cc @@ -316,7 +316,7 @@ void WriteLog::enlist_op_appender() { * all prior log entries are persisted everywhere. */ template -void WriteLog::schedule_append_ops(GenericLogOperations &ops) { +void WriteLog::schedule_append_ops(GenericLogOperations &ops, C_BlockIORequestT *req) { bool need_finisher = false; GenericLogOperationsVector appending; @@ -334,6 +334,17 @@ void WriteLog::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 void WriteLog::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 diff --git a/src/librbd/cache/pwl/ssd/WriteLog.h b/src/librbd/cache/pwl/ssd/WriteLog.h index 1a7a5e7a11629..f71b6d5ab11da 100644 --- 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 log_entry) override; -- 2.39.5