osd/ReplicatedBackend: fix use-after-free on InProgressOp

- op in flight to disk...
- on_change() clears the InProgressOp
- C_OSD_OnOpCommit calls op_commit() w/ bare pointer
- crash!

Fix by refcounting InProgressOp and clearing on_commit when it is
canceled.

Fixes: http://tracker.ceph.com/issues/24219
Signed-off-by: Sage Weil <sage@redhat.com>

diff --git a/src/osd/ReplicatedBackend.cc b/src/osd/ReplicatedBackend.cc
index 013682db9d8e4d91874a25f6d8acc5ac919d61b6..8b15791748090cc6b34ac69dcebf765786f3e64d 100644 (file)
--- a/src/osd/ReplicatedBackend.cc
+++ b/src/osd/ReplicatedBackend.cc
@@ -236,7 +236,8 @@ void ReplicatedBackend::on_change()
 {
   dout(10) << __func__ << dendl;
   for (auto& op : in_progress_ops) {
-    delete op.second.on_commit;
+    delete op.second->on_commit;
+    op.second->on_commit = nullptr;
   }
   in_progress_ops.clear();
   clear_recovery_state();
@@ -264,7 +265,7 @@ void ReplicatedBackend::objects_read_async(
 
 class C_OSD_OnOpCommit : public Context {
   ReplicatedBackend *pg;
-  ReplicatedBackend::InProgressOp *op;
+  ReplicatedBackend::InProgressOpRef op;
 public:
   C_OSD_OnOpCommit(ReplicatedBackend *pg, ReplicatedBackend::InProgressOp *op) 
     : pg(pg), op(op) {}
@@ -453,13 +454,13 @@ void ReplicatedBackend::submit_transaction(
   auto insert_res = in_progress_ops.insert(
     make_pair(
       tid,
-      InProgressOp(
+      new InProgressOp(
        tid, on_all_commit,
        orig_op, at_version)
       )
     );
   assert(insert_res.second);
-  InProgressOp &op = insert_res.first->second;
+  InProgressOp &op = *insert_res.first->second;
 
   op.waiting_for_commit.insert(
     parent->get_acting_recovery_backfill_shards().begin(),
@@ -504,8 +505,13 @@ void ReplicatedBackend::submit_transaction(
 }
 
 void ReplicatedBackend::op_commit(
-  InProgressOp *op)
+  InProgressOpRef& op)
 {
+  if (op->on_commit == nullptr) {
+    // aborted
+    return;
+  }
+
   FUNCTRACE(cct);
   OID_EVENT_TRACE_WITH_MSG((op && op->op) ? op->op->get_req() : NULL, "OP_COMMIT_BEGIN", true);
   dout(10) << __func__ << ": " << op->tid << dendl;
@@ -538,7 +544,7 @@ void ReplicatedBackend::do_repop_reply(OpRequestRef op)
 
   auto iter = in_progress_ops.find(rep_tid);
   if (iter != in_progress_ops.end()) {
-    InProgressOp &ip_op = iter->second;
+    InProgressOp &ip_op = *iter->second;
     const MOSDOp *m = NULL;
     if (ip_op.op)
       m = static_cast<const MOSDOp *>(ip_op.op->get_req());

diff --git a/src/osd/ReplicatedBackend.h b/src/osd/ReplicatedBackend.h
index a54f19933cac7e5e6b0ae5ff4d20b77231777989..adfbaa1b794ab7b8571ff24ec9c794b8341994ad 100644 (file)
--- a/src/osd/ReplicatedBackend.h
+++ b/src/osd/ReplicatedBackend.h
@@ -327,7 +327,7 @@ private:
   /**
    * Client IO
    */
-  struct InProgressOp {
+  struct InProgressOp : public RefCountedObject {
     ceph_tid_t tid;
     set<pg_shard_t> waiting_for_commit;
     Context *on_commit;
@@ -336,13 +336,15 @@ private:
     InProgressOp(
       ceph_tid_t tid, Context *on_commit,
       OpRequestRef op, eversion_t v)
-      : tid(tid), on_commit(on_commit),
+      : RefCountedObject(nullptr, 0),
+       tid(tid), on_commit(on_commit),
        op(op), v(v) {}
     bool done() const {
       return waiting_for_commit.empty();
     }
   };
-  map<ceph_tid_t, InProgressOp> in_progress_ops;
+  typedef boost::intrusive_ptr<InProgressOp> InProgressOpRef;
+  map<ceph_tid_t, InProgressOpRef> in_progress_ops;
 public:
   friend class C_OSD_OnOpCommit;
 
@@ -395,7 +397,7 @@ private:
     boost::optional<pg_hit_set_history_t> &hset_history,
     InProgressOp *op,
     ObjectStore::Transaction &op_t);
-  void op_commit(InProgressOp *op);
+  void op_commit(InProgressOpRef& op);
   void do_repop_reply(OpRequestRef op);
   void do_repop(OpRequestRef op);