From: Sage Weil Date: Mon, 19 Apr 2021 14:26:30 +0000 (-0500) Subject: msgr/async: fix unsafe access in unregister_conn() X-Git-Tag: v15.2.16~80^2 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=2cb463b915c76db4ed74a342d5795c0ff2b263e6;p=ceph.git 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 (cherry picked from commit d51d80b3234e17690061f65dc7e1515f4244a5a3) Signed-off-by: Gerald Yang --- diff --git a/src/msg/async/AsyncMessenger.cc b/src/msg/async/AsyncMessenger.cc index 7595b35d77c21..54e4a69f63acc 100644 --- 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()) { - 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 3ce1b61ce6047..d2dbda9cd2d34 100644 --- 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();