msgr/async: fix unsafe access in unregister_conn()

We were looking at anon_conns and accepting_conns without holding
the lock (deleted_lock is not sufficient).

Drop this test, and move the decrements:

- inc when we add to conns or anon_conns (no changes there)
- dec when we remove from deleted_conns (several different paths!)

Fixes: https://tracker.ceph.com/issues/49237
Signed-off-by: Sage Weil <sage@newdream.net>

diff --git a/src/msg/async/AsyncMessenger.cc b/src/msg/async/AsyncMessenger.cc
index 8e98ef49313ef9e3dd4d4fddb3463027d17d394c..f5dd03295e92add48cf3dd88e199c33e9589ce2b 100644 (file)
--- a/src/msg/async/AsyncMessenger.cc
+++ b/src/msg/async/AsyncMessenger.cc
@@ -788,24 +788,21 @@ void AsyncMessenger::shutdown_connections(bool queue_reset)
 
   for (const auto& [e, c] : conns) {
     ldout(cct, 5) << __func__ << " mark down " << e << " " << c << dendl;
-    c->get_perf_counter()->dec(l_msgr_active_connections);
     c->stop(queue_reset);
   }
   conns.clear();
 
   for (const auto& c : anon_conns) {
     ldout(cct, 5) << __func__ << " mark down " << c << dendl;
-    c->get_perf_counter()->dec(l_msgr_active_connections);
     c->stop(queue_reset);
   }
   anon_conns.clear();
 
   {
     std::lock_guard l{deleted_lock};
-    if (cct->_conf->subsys.should_gather<ceph_subsys_ms, 5>()) {
-      for (const auto& c : deleted_conns) {
-        ldout(cct, 5) << __func__ << " delete " << c << dendl;
-      }
+    for (const auto& c : deleted_conns) {
+      ldout(cct, 5) << __func__ << " delete " << c << dendl;
+      c->get_perf_counter()->dec(l_msgr_active_connections);
     }
     deleted_conns.clear();
   }
@@ -860,6 +857,7 @@ int AsyncMessenger::accept_conn(const AsyncConnectionRef& conn)
     // If conn already in, we will return 0
     std::lock_guard l{deleted_lock};
     if (deleted_conns.erase(existing)) {
+      it->second->get_perf_counter()->dec(l_msgr_active_connections);
       conns.erase(it);
     } else if (conn != existing) {
       return -1;
@@ -939,6 +937,7 @@ void AsyncMessenger::reap_dead()
         conns.erase(conns_it);
       accepting_conns.erase(c);
       anon_conns.erase(c);
+      c->get_perf_counter()->dec(l_msgr_active_connections);
     }
     deleted_conns.clear();
   }

diff --git a/src/msg/async/AsyncMessenger.h b/src/msg/async/AsyncMessenger.h
index 625cb099c5a6f8f131082ee74734b5695ceb7558..00bed684cd34c04ee305912b214e4873880246c5 100644 (file)
--- a/src/msg/async/AsyncMessenger.h
+++ b/src/msg/async/AsyncMessenger.h
@@ -311,6 +311,7 @@ private:
     if (p->second->is_unregistered()) {
       std::lock_guard l{deleted_lock};
       if (deleted_conns.erase(p->second)) {
+       p->second->get_perf_counter()->dec(l_msgr_active_connections);
        conns.erase(p);
        return nullref;
       }
@@ -399,8 +400,6 @@ public:
    */
   void unregister_conn(const AsyncConnectionRef& conn) {
     std::lock_guard l{deleted_lock};
-    if (!accepting_conns.count(conn) || anon_conns.count(conn))
-      conn->get_perf_counter()->dec(l_msgr_active_connections);
     deleted_conns.emplace(std::move(conn));
     conn->unregister();