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 ceph#11 ... ``` Signed-off-by: Radoslaw Zarzynski <[email protected]>
tchaikov · Jul 5, 2022 · 2951088 · 2951088
1 parent de70e12
commit 2951088
Show file tree

Hide file tree

Showing 12 changed files with 47 additions and 7 deletions.
diff --git a/src/crimson/os/seastore/ordering_handle.h b/src/crimson/os/seastore/ordering_handle.h
@@ -54,8 +54,12 @@ class PlaceholderOperation : public crimson::osd::PhasedOperationT<PlaceholderOp
     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
@@ -151,6 +151,14 @@ class TrackableOperationT : public OperationT<T> {
 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 @@ class PhasedOperationT : public TrackableOperationT<T> {
     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
@@ -114,6 +114,7 @@ class BackfillRecovery final : public BackgroundRecoveryT<BackfillRecovery> {
     const EventT& evt);
 
   static BackfillRecoveryPipeline &bp(PG &pg);
+  PipelineHandle& get_handle() { return handle; }
 
   std::tuple<
     OperationThrottler::BlockingEvent,
@@ -123,6 +124,7 @@ class BackfillRecovery final : public BackgroundRecoveryT<BackfillRecovery> {
 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
@@ -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
@@ -45,8 +45,11 @@ class InternalClientRequest : public PhasedOperationT<InternalClientRequest>,
 
   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
@@ -62,6 +62,8 @@ class LogMissingRequest final : public PhasedOperationT<LogMissingRequest> {
   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
@@ -62,6 +62,8 @@ class LogMissingRequestReply final : public PhasedOperationT<LogMissingRequestRe
   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
@@ -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
@@ -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 @@ class PeeringEvent : public PhasedOperationT<T> {
 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 @@ class RemotePeeringEvent : public PeeringEvent<RemotePeeringEvent> {
 class LocalPeeringEvent final : public PeeringEvent<LocalPeeringEvent> {
 protected:
   Ref<PG> pg;
+  PipelineHandle handle;
 
 public:
   template <typename... Args>
@@ -174,6 +183,8 @@ class LocalPeeringEvent final : public PeeringEvent<LocalPeeringEvent> {
   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
@@ -27,6 +27,7 @@ class PGAdvanceMap : public PhasedOperationT<PGAdvanceMap> {
 protected:
   OSD &osd;
   Ref<PG> pg;
+  PipelineHandle handle;
 
   epoch_t from;
   epoch_t to;
@@ -43,6 +44,7 @@ class PGAdvanceMap : public PhasedOperationT<PGAdvanceMap> {
   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
@@ -56,6 +56,8 @@ class RecoverySubRequest final : public PhasedOperationT<RecoverySubRequest> {
 
 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
@@ -61,6 +61,7 @@ class RepRequest final : public PhasedOperationT<RepRequest> {
   PGPipeline &pp(PG &pg);
 
   crimson::net::ConnectionRef conn;
+  PipelineHandle handle;
   Ref<MOSDRepOp> req;
 };