From 295108881fdf36f4885e731a4ae5da7ae03ddf9c Mon Sep 17 00:00:00 2001 From: Radoslaw Zarzynski Date: Tue, 5 Jul 2022 18:16:47 +0000 Subject: [PATCH] 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::exit() ../src/crimson/common/operation.h:548 #2 0x20c184e in crimson::OrderedExclusivePhaseT::ExitBarrier::exit() ../src/crimson/common/operation.h:533 #3 0x20c184e in crimson::OrderedExclusivePhaseT::ExitBarrier::cancel() ../src/crimson/common/operation.h:539 #4 0x20c184e in crimson::OrderedExclusivePhaseT::ExitBarrier::~ExitBarrier() ../src/crimson/common/operation.h:543 #5 0x20c184e in crimson::OrderedExclusivePhaseT::ExitBarrier::~ExitBarrier() ../src/crimson/common/operation.h:544 #6 0x352eb7f in std::default_delete::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 >::~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::~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 --- src/crimson/os/seastore/ordering_handle.h | 4 ++++ src/crimson/osd/osd_operation.h | 15 ++++++++++++--- .../osd/osd_operations/background_recovery.h | 2 ++ src/crimson/osd/osd_operations/client_request.h | 4 +++- .../osd/osd_operations/internal_client_request.h | 3 +++ .../osd/osd_operations/logmissing_request.h | 2 ++ .../osd/osd_operations/logmissing_request_reply.h | 2 ++ src/crimson/osd/osd_operations/peering_event.cc | 4 ++-- src/crimson/osd/osd_operations/peering_event.h | 13 ++++++++++++- src/crimson/osd/osd_operations/pg_advance_map.h | 2 ++ .../osd/osd_operations/recovery_subrequest.h | 2 ++ .../osd/osd_operations/replicated_request.h | 1 + 12 files changed, 47 insertions(+), 7 deletions(-) diff --git a/src/crimson/os/seastore/ordering_handle.h b/src/crimson/os/seastore/ordering_handle.h index add75066be4..a7802fda383 100644 --- 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 323f7bb5939..37da9c5633f 100644 --- a/src/crimson/osd/osd_operation.h +++ b/src/crimson/osd/osd_operation.h @@ -151,6 +151,14 @@ protected: template class PhasedOperationT : public TrackableOperationT { using base_t = TrackableOperationT; + + T* that() { + return static_cast(this); + } + const T* that() const { + return static_cast(this); + } + protected: using TrackableOperationT::TrackableOperationT; @@ -159,12 +167,13 @@ protected: return this->template with_blocking_event( [&stage, this] (auto&& trigger) { - return handle.enter(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(stage, std::move(trigger)); }); } - PipelineHandle handle; - template 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 1e5ffb29d0f..aef7135f9bf 100644 --- 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 evt; PipelineHandle handle; + interruptible_future do_recovery() override; }; diff --git a/src/crimson/osd/osd_operations/client_request.h b/src/crimson/osd/osd_operations/client_request.h index 82ccbe5f38f..365420b4ecb 100644 --- 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, 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 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 6ad77121df0..f1773cb8dd8 100644 --- 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; 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 6fad844b678..ba6defcd3a3 100644 --- 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 req; }; diff --git a/src/crimson/osd/osd_operations/logmissing_request_reply.h b/src/crimson/osd/osd_operations/logmissing_request_reply.h index 5632bb4ab7c..cadec4a9acc 100644 --- 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 req; }; diff --git a/src/crimson/osd/osd_operations/peering_event.cc b/src/crimson/osd/osd_operations/peering_event.cc index 0b26833fd75..1347a2dc684 100644 --- 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::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::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 5c1b707c8b6..65b9e9e7963 100644 --- 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 PeeringEvent : public PhasedOperationT { + T* that() { + return static_cast(this); + } + const T* that() const { + return static_cast(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 { 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) override; @@ -163,6 +171,7 @@ public: class LocalPeeringEvent final : public PeeringEvent { protected: Ref pg; + PipelineHandle handle; public: template @@ -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 1bb53f0dc89..1ec23029b2d 100644 --- 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; + 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 7c4d106712d..e629b055faa 100644 --- 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 m; }; diff --git a/src/crimson/osd/osd_operations/replicated_request.h b/src/crimson/osd/osd_operations/replicated_request.h index f84b107721a..33ea8a86ca1 100644 --- 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 req; }; -- 2.39.5