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 add75066be4d1..a7802fda383d9 100644 --- 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 { 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 @@ class PhasedOperationT : public TrackableOperationT { 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 1e5ffb29d0f30..aef7135f9bf2d 100644 --- 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 { 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 { 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 82ccbe5f38f3b..365420b4ecbce 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 6ad77121df07c..f1773cb8dd872 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 @@ class InternalClientRequest : public PhasedOperationT, 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 6fad844b67808..ba6defcd3a3c5 100644 --- 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 { 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 5632bb4ab7ca5..cadec4a9acc71 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 @@ class LogMissingRequestReply final : public PhasedOperationT req; }; diff --git a/src/crimson/osd/osd_operations/peering_event.cc b/src/crimson/osd/osd_operations/peering_event.cc index 0b26833fd75c3..1347a2dc68490 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 5c1b707c8b612..65b9e9e79633d 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 @@ class PeeringEvent : public PhasedOperationT { 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 @@ class RemotePeeringEvent : public PeeringEvent { class LocalPeeringEvent final : public PeeringEvent { protected: Ref pg; + PipelineHandle handle; public: template @@ -174,6 +183,8 @@ class LocalPeeringEvent final : public PeeringEvent { 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 1bb53f0dc8998..1ec23029b2d97 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 @@ class PGAdvanceMap : public PhasedOperationT { protected: OSD &osd; Ref pg; + PipelineHandle handle; epoch_t from; epoch_t to; @@ -43,6 +44,7 @@ class PGAdvanceMap : public PhasedOperationT { 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 7c4d106712d16..e629b055faade 100644 --- 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 { 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 f84b107721ac5..33ea8a86ca129 100644 --- 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 { PGPipeline &pp(PG &pg); crimson::net::ConnectionRef conn; + PipelineHandle handle; Ref req; };