crimson/osd: fix life-time management of OSDConnectionPriv

Before the patch there was a possibility that `OSDConnectionPriv`
gets destructed before a `PipelineHandle` instance that was using
it. The reason is our remote-handling operations store `conn` directly
while `handle` is defined in a parent class. Due to the language rules
the former gets deinitialized earlier.

```
==756032==ERROR: AddressSanitizer: heap-use-after-free on address 0x615000039684 at pc 0x0000020bdfa2 bp 0x7ffd3abfa370 sp 0x7ffd3abfa360
READ of size 1 at 0x615000039684 thread T0
Reactor stalled for 261 ms on shard 0. Backtrace: 0x45d9d 0xe90f6d1 0xe6b8a1d 0xe6d1205 0xe6d16a8 0xe6d1938 0xe6d1c03 0x12cdf 0xccebf 0x7f6447161b1e 0x7f644714aee8 0x7f644714eed6 0x7f644714fb36 0x7f64471420b5 0x
7f6447143f3a 0xd61d0 0x32412 0xbd8a7 0xbd134 0xbdc1a 0x20bdfa1 0x20c184e 0x352eb7f 0x352fa28 0x20b04a5 0x1be30e5 0xe694bc4 0xe6ebb8a 0xe843a11 0xe845a22 0xe29f497 0xe2a3ccd 0x1ab1841 0x3aca2 0x175698d
    #0 0x20bdfa1 in seastar::shared_mutex::unlock() ../src/seastar/include/seastar/core/shared_mutex.hh:122
    #1 0x20c184e in crimson::OrderedExclusivePhaseT<crimson::osd::ConnectionPipeline::GetPG>::exit() ../src/crimson/common/operation.h:548
    #2 0x20c184e in crimson::OrderedExclusivePhaseT<crimson::osd::ConnectionPipeline::GetPG>::ExitBarrier::exit() ../src/crimson/common/operation.h:533
    #3 0x20c184e in crimson::OrderedExclusivePhaseT<crimson::osd::ConnectionPipeline::GetPG>::ExitBarrier::cancel() ../src/crimson/common/operation.h:539
    #4 0x20c184e in crimson::OrderedExclusivePhaseT<crimson::osd::ConnectionPipeline::GetPG>::ExitBarrier::~ExitBarrier() ../src/crimson/common/operation.h:543
    #5 0x20c184e in crimson::OrderedExclusivePhaseT<crimson::osd::ConnectionPipeline::GetPG>::ExitBarrier::~ExitBarrier() ../src/crimson/common/operation.h:544
    #6 0x352eb7f in std::default_delete<crimson::PipelineExitBarrierI>::operator()(crimson::PipelineExitBarrierI*) const /opt/rh/gcc-toolset-11/root/usr/include/c++/11/bits/unique_ptr.h:85
    #7 0x352eb7f in std::unique_ptr<crimson::PipelineExitBarrierI, std::default_delete<crimson::PipelineExitBarrierI> >::~unique_ptr() /opt/rh/gcc-toolset-11/root/usr/include/c++/11/bits/unique_ptr.h:361
    #8 0x352eb7f in crimson::PipelineHandle::~PipelineHandle() ../src/crimson/common/operation.h:457
    #9 0x352eb7f in crimson::osd::PhasedOperationT<crimson::osd::ClientRequest>::~PhasedOperationT() ../src/crimson/osd/osd_operation.h:152
    #10 0x352eb7f in crimson::osd::ClientRequest::~ClientRequest() ../src/crimson/osd/osd_operations/client_request.cc:64
    #11 ...
```

Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>

diff --git a/src/crimson/os/seastore/ordering_handle.h b/src/crimson/os/seastore/ordering_handle.h
index add75066be4d139b1b15ddafdf6ffe1369f8be79..a7802fda383d97dac9085b566774b5d26f7c232c 100644 (file)
--- a/src/crimson/os/seastore/ordering_handle.h
+++ b/src/crimson/os/seastore/ordering_handle.h
@@ -54,8 +54,12 @@ public:
     return IRef{new PlaceholderOperation()};
   }
 
+  PipelineHandle handle;
   WritePipeline::BlockingEvents tracking_events;
 
+  PipelineHandle& get_handle() {
+    return handle;
+  }
 private:
   void dump_detail(ceph::Formatter *f) const final {}
   void print(std::ostream &) const final {}

diff --git a/src/crimson/osd/osd_operation.h b/src/crimson/osd/osd_operation.h
index 323f7bb59397ce8e4c2077db8b693157f7bc842c..37da9c5633fdbcc17c2e8992a5f86fd9e9d9e930 100644 (file)
--- a/src/crimson/osd/osd_operation.h
+++ b/src/crimson/osd/osd_operation.h
@@ -151,6 +151,14 @@ protected:
 template <class T>
 class PhasedOperationT : public TrackableOperationT<T> {
   using base_t = TrackableOperationT<T>;
+
+  T* that() {
+    return static_cast<T*>(this);
+  }
+  const T* that() const {
+    return static_cast<const T*>(this);
+  }
+
 protected:
   using TrackableOperationT<T>::TrackableOperationT;
 
@@ -159,12 +167,13 @@ protected:
     return this->template with_blocking_event<typename StageT::BlockingEvent,
                                              InterruptorT>(
       [&stage, this] (auto&& trigger) {
-        return handle.enter<T>(stage, std::move(trigger));
+        // delegated storing the pipeline handle to let childs to match
+        // the lifetime of pipeline with e.g. ConnectedSocket (important
+        // for ConnectionPipeline).
+        return that()->get_handle().template enter<T>(stage, std::move(trigger));
     });
   }
 
-  PipelineHandle handle;
-
   template <class OpT>
   friend class crimson::os::seastore::OperationProxyT;
 

diff --git a/src/crimson/osd/osd_operations/background_recovery.h b/src/crimson/osd/osd_operations/background_recovery.h
index 1e5ffb29d0f309506b95c2a5e4d30725a2f67b99..aef7135f9bf2da4ab9bb26cdbb3b1731e9e6fbc0 100644 (file)
--- a/src/crimson/osd/osd_operations/background_recovery.h
+++ b/src/crimson/osd/osd_operations/background_recovery.h
@@ -114,6 +114,7 @@ public:
     const EventT& evt);
 
   static BackfillRecoveryPipeline &bp(PG &pg);
+  PipelineHandle& get_handle() { return handle; }
 
   std::tuple<
     OperationThrottler::BlockingEvent,
@@ -123,6 +124,7 @@ public:
 private:
   boost::intrusive_ptr<const boost::statechart::event_base> evt;
   PipelineHandle handle;
+
   interruptible_future<bool> do_recovery() override;
 };
 

diff --git a/src/crimson/osd/osd_operations/client_request.h b/src/crimson/osd/osd_operations/client_request.h
index 82ccbe5f38f3bc44fc4b69fdb711638be4b454ea..365420b4ecbce079afd1b2cf4dac8c0478469772 100644 (file)
--- a/src/crimson/osd/osd_operations/client_request.h
+++ b/src/crimson/osd/osd_operations/client_request.h
@@ -28,7 +28,9 @@ class ShardServices;
 class ClientRequest final : public PhasedOperationT<ClientRequest>,
                             private CommonClientRequest {
   OSD &osd;
-  crimson::net::ConnectionRef conn;
+  const crimson::net::ConnectionRef conn;
+  // must be after conn due to ConnectionPipeline's life-time
+  PipelineHandle handle;
   Ref<MOSDOp> m;
   OpInfo op_info;
   seastar::promise<> on_complete;

diff --git a/src/crimson/osd/osd_operations/internal_client_request.h b/src/crimson/osd/osd_operations/internal_client_request.h
index 6ad77121df07cc2b76ac18920321879784c3468b..f1773cb8dd8721e7e23c8a482aaf99ff520d3dfa 100644 (file)
--- a/src/crimson/osd/osd_operations/internal_client_request.h
+++ b/src/crimson/osd/osd_operations/internal_client_request.h
@@ -45,8 +45,11 @@ private:
 
   Ref<PG> pg;
   OpInfo op_info;
+  PipelineHandle handle;
 
 public:
+  PipelineHandle& get_handle() { return handle; }
+
   std::tuple<
     StartEvent,
     CommonPGPipeline::WaitForActive::BlockingEvent,

diff --git a/src/crimson/osd/osd_operations/logmissing_request.h b/src/crimson/osd/osd_operations/logmissing_request.h
index 6fad844b6780845276098f9c79728e8dd1af25d0..ba6defcd3a3c5153d09978adb1a6aac63f601487 100644 (file)
--- a/src/crimson/osd/osd_operations/logmissing_request.h
+++ b/src/crimson/osd/osd_operations/logmissing_request.h
@@ -62,6 +62,8 @@ private:
   RepRequest::PGPipeline &pp(PG &pg);
 
   crimson::net::ConnectionRef conn;
+  // must be after `conn` to ensure the ConnectionPipeline's is alive
+  PipelineHandle handle;
   Ref<MOSDPGUpdateLogMissing> req;
 };
 

diff --git a/src/crimson/osd/osd_operations/logmissing_request_reply.h b/src/crimson/osd/osd_operations/logmissing_request_reply.h
index 5632bb4ab7ca55387a525f1bef8ef98b43957d91..cadec4a9acc715d115956c4e4090eda57f574a98 100644 (file)
--- a/src/crimson/osd/osd_operations/logmissing_request_reply.h
+++ b/src/crimson/osd/osd_operations/logmissing_request_reply.h
@@ -62,6 +62,8 @@ private:
   RepRequest::PGPipeline &pp(PG &pg);
 
   crimson::net::ConnectionRef conn;
+  // must be after `conn` to ensure the ConnectionPipeline's is alive
+  PipelineHandle handle;
   Ref<MOSDPGUpdateLogMissingReply> req;
 };
 

diff --git a/src/crimson/osd/osd_operations/peering_event.cc b/src/crimson/osd/osd_operations/peering_event.cc
index 0b26833fd75c3f8088f670b05296d65d1affc427..1347a2dc68490fb203a8bb9dea16e03fda7912c3 100644 (file)
--- a/src/crimson/osd/osd_operations/peering_event.cc
+++ b/src/crimson/osd/osd_operations/peering_event.cc
@@ -59,7 +59,7 @@ seastar::future<> PeeringEvent<T>::with_pg(
   if (!pg) {
     logger().warn("{}: pg absent, did not create", *this);
     on_pg_absent();
-    handle.exit();
+    that()->get_handle().exit();
     return complete_rctx_no_pg();
   }
 
@@ -83,7 +83,7 @@ seastar::future<> PeeringEvent<T>::with_pg(
        BackfillRecovery::bp(*pg).process);
     }).then_interruptible([this, pg] {
       pg->do_peering_event(evt, ctx);
-      handle.exit();
+      that()->get_handle().exit();
       return complete_rctx(pg);
     }).then_interruptible([pg, &shard_services]()
                          -> typename T::template interruptible_future<> {

diff --git a/src/crimson/osd/osd_operations/peering_event.h b/src/crimson/osd/osd_operations/peering_event.h
index 5c1b707c8b612f1f1a76d2cbeb99c80a1f7b4d8c..65b9e9e79633d5cc5bc4781ce5495dd836de731d 100644 (file)
--- a/src/crimson/osd/osd_operations/peering_event.h
+++ b/src/crimson/osd/osd_operations/peering_event.h
@@ -39,11 +39,17 @@ class PG;
 
 template <class T>
 class PeeringEvent : public PhasedOperationT<T> {
+  T* that() {
+    return static_cast<T*>(this);
+  }
+  const T* that() const {
+    return static_cast<const T*>(this);
+  }
+
 public:
   static constexpr OperationTypeCode type = OperationTypeCode::peering_event;
 
 protected:
-  PipelineHandle handle;
   PGPeeringPipeline &pp(PG &pg);
 
   ShardServices &shard_services;
@@ -102,6 +108,8 @@ public:
 class RemotePeeringEvent : public PeeringEvent<RemotePeeringEvent> {
 protected:
   crimson::net::ConnectionRef conn;
+  // must be after conn due to ConnectionPipeline's life-time
+  PipelineHandle handle;
 
   void on_pg_absent() final;
   PeeringEvent::interruptible_future<> complete_rctx(Ref<PG> pg) override;
@@ -163,6 +171,7 @@ public:
 class LocalPeeringEvent final : public PeeringEvent<LocalPeeringEvent> {
 protected:
   Ref<PG> pg;
+  PipelineHandle handle;
 
 public:
   template <typename... Args>
@@ -174,6 +183,8 @@ public:
   seastar::future<> start();
   virtual ~LocalPeeringEvent();
 
+  PipelineHandle &get_handle() { return handle; }
+
   std::tuple<
     StartEvent,
     PGPeeringPipeline::AwaitMap::BlockingEvent,

diff --git a/src/crimson/osd/osd_operations/pg_advance_map.h b/src/crimson/osd/osd_operations/pg_advance_map.h
index 1bb53f0dc8998f840322c78f91ea75846a78f613..1ec23029b2d976117a7cc17e337fac53638ac9dd 100644 (file)
--- a/src/crimson/osd/osd_operations/pg_advance_map.h
+++ b/src/crimson/osd/osd_operations/pg_advance_map.h
@@ -27,6 +27,7 @@ public:
 protected:
   OSD &osd;
   Ref<PG> pg;
+  PipelineHandle handle;
 
   epoch_t from;
   epoch_t to;
@@ -43,6 +44,7 @@ public:
   void print(std::ostream &) const final;
   void dump_detail(ceph::Formatter *f) const final;
   seastar::future<> start();
+  PipelineHandle &get_handle() { return handle; }
 
   std::tuple<
     PGPeeringPipeline::Process::BlockingEvent

diff --git a/src/crimson/osd/osd_operations/recovery_subrequest.h b/src/crimson/osd/osd_operations/recovery_subrequest.h
index 7c4d106712d16367238cd7889e9e2ab0909ebaee..e629b055faade865e67a784f473c0b07529674ea 100644 (file)
--- a/src/crimson/osd/osd_operations/recovery_subrequest.h
+++ b/src/crimson/osd/osd_operations/recovery_subrequest.h
@@ -56,6 +56,8 @@ public:
 
 private:
   crimson::net::ConnectionRef conn;
+  // must be after `conn` to ensure the ConnectionPipeline's is alive
+  PipelineHandle handle;
   Ref<MOSDFastDispatchOp> m;
 };
 

diff --git a/src/crimson/osd/osd_operations/replicated_request.h b/src/crimson/osd/osd_operations/replicated_request.h
index f84b107721ac5e5fe8c1b5fc221fadf35e35a37a..33ea8a86ca129bb58cf4ee9af4f1610abe719573 100644 (file)
--- a/src/crimson/osd/osd_operations/replicated_request.h
+++ b/src/crimson/osd/osd_operations/replicated_request.h
@@ -61,6 +61,7 @@ private:
   PGPipeline &pp(PG &pg);
 
   crimson::net::ConnectionRef conn;
+  PipelineHandle handle;
   Ref<MOSDRepOp> req;
 };