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>
(cherry picked from commit d51d80b3234e17690061f65dc7e1515f4244a5a3)
Signed-off-by: Gerald Yang <gerald.yang.tw@gmail.com>

diff --git a/src/msg/async/AsyncMessenger.cc b/src/msg/async/AsyncMessenger.cc
index 7595b35d77c2104802c40045afac129846510383..54e4a69f63accdf6178ba650536b9512f9e14dde 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 3ce1b61ce604784663ac3ae743bd2e67313ed940..d2dbda9cd2d34b9e0193319e69659642a4963dae 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();