msg/async: mark down local_connection before draining the stack.

`AsyncMessenger` has `local_connection` – an instance of
`AsyncConnection` used to send messages to itself (loop back).
Connections are handled by `EventCenter` in its dedicated
thread. When shutting down a messenger, it must be ensured
there is no task left in the `EventCenter`'s queue. Otherwise
a use-after-free can happen. That's the reason why `shutdown()`
of `AsyncMessenger` does `mark_down_all()` on connections
**before** draining the stack. The latter actually injects a task
into all `EventCenter` instances and waits for its completion
(synchronization barrier).

However, that's not the case for `local_connection`. Without
the patch it's marked down by the messenger's destructor
far **after** the synchronization barrier. This behavior is
dangerous when an implementation of `mark_down()` creates
a new task to be executed inside the boundaries of corresponding
`EventCenter` instance.

The fix unifies handling of `local_connection` with other
connections in the aspect of the shutdown phase.

Fixes: https://tracker.ceph.com/issues/43667
Signed-off-by: Radoslaw Zarzynski <rzarzyns@redhat.com>
(cherry picked from commit c2f61b95f50f88bfbfe4f46fcc447762c119bae1)

diff --git a/src/msg/async/AsyncMessenger.cc b/src/msg/async/AsyncMessenger.cc
index 4517cbaad5fb73eeb98423921cf5ba743c5dc0bc..75a99ac854203931042377f8f42d7448c85c8e3b 100644 (file)
--- a/src/msg/async/AsyncMessenger.cc
+++ b/src/msg/async/AsyncMessenger.cc
@@ -310,7 +310,6 @@ AsyncMessenger::~AsyncMessenger()
 {
   delete reap_handler;
   ceph_assert(!did_bind); // either we didn't bind or we shut down the Processor
-  local_connection->mark_down();
   for (auto &&p : processors)
     delete p;
 }
@@ -344,6 +343,7 @@ int AsyncMessenger::shutdown()
   mark_down_all();
   // break ref cycles on the loopback connection
   local_connection->set_priv(NULL);
+  local_connection->mark_down();
   did_bind = false;
   lock.Lock();
   stop_cond.Signal();