From 2375d060871cc95a9c64e0b3478a528009f2edaf Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Thu, 23 Aug 2018 13:47:12 +0800 Subject: [PATCH] auth: drop the RWLock in AuthClientHandler the `RWLock` in `AuthClientHandler` was introduced in #1636. back to then, we were accessing `MonClient::auth` directly, see https://github.com/ceph/ceph/commit/a2eb6ae3fb57b09efdd4d7baac6871ca8dd8e79f#diff-fa6c2eba8356ae1442d1bf749beacfdfL6209 . and in #11128, we added a helper method offering accessing to `MonClient::auth`. this method guards the access to `auth` using `monc_lock`. and there is no other users outside of `MonClient` using `AuthClientHandler`. and all accesses to `MonClient::auth` are now protected by `monc_lock`. Signed-off-by: Kefu Chai --- src/auth/AuthClientHandler.cc | 14 +++++++++----- src/auth/AuthClientHandler.h | 10 +++------- src/auth/cephx/CephxClientHandler.cc | 6 ------ src/auth/cephx/CephxClientHandler.h | 13 ++----------- src/auth/none/AuthNoneClientHandler.h | 13 +++---------- src/mon/MonClient.cc | 2 +- src/mon/MonClient.h | 8 ++++---- src/mon/Monitor.cc | 2 +- 8 files changed, 23 insertions(+), 45 deletions(-) diff --git a/src/auth/AuthClientHandler.cc b/src/auth/AuthClientHandler.cc index 320d2faba4e..1ac96a50c4f 100644 --- a/src/auth/AuthClientHandler.cc +++ b/src/auth/AuthClientHandler.cc @@ -21,19 +21,23 @@ template -AuthClientHandler* -AuthClientHandler::create(CephContext *cct, int proto, - RotatingKeyRing *rkeys) +AuthClientHandler* +AuthClientHandler::create(CephContext *cct, int proto, + RotatingKeyRing *rkeys) { switch (proto) { case CEPH_AUTH_CEPHX: return new CephxClientHandler(cct, rkeys); case CEPH_AUTH_NONE: - return new AuthNoneClientHandler(cct, rkeys); + return new AuthNoneClientHandler{cct}; default: return NULL; } } // explicitly instantiate only the classes we need -template class AuthClientHandler; +template AuthClientHandler* +AuthClientHandler::create( + CephContext *cct, + int proto, + RotatingKeyRing *rkeys); diff --git a/src/auth/AuthClientHandler.h b/src/auth/AuthClientHandler.h index 140dbcc63b4..a395c326838 100644 --- a/src/auth/AuthClientHandler.h +++ b/src/auth/AuthClientHandler.h @@ -18,13 +18,11 @@ #include "auth/Auth.h" #include "common/lock_policy.h" -#include "common/lock_shared_mutex.h" class CephContext; struct MAuthReply; template class RotatingKeyRing; -template class AuthClientHandler { protected: CephContext *cct; @@ -33,19 +31,16 @@ protected: uint32_t want; uint32_t have; uint32_t need; - mutable SharedMutexT lock; public: explicit AuthClientHandler(CephContext *cct_) - : cct(cct_), global_id(0), want(CEPH_ENTITY_TYPE_AUTH), have(0), need(0), - lock{SharedMutex::create("AuthClientHandler::lock")} + : cct(cct_), global_id(0), want(CEPH_ENTITY_TYPE_AUTH), have(0), need(0) {} virtual ~AuthClientHandler() {} void init(const EntityName& n) { name = n; } void set_want_keys(__u32 keys) { - std::unique_lock l{lock}; want = keys | CEPH_ENTITY_TYPE_AUTH; validate_tickets(); } @@ -64,7 +59,8 @@ public: virtual void set_global_id(uint64_t id) = 0; - static AuthClientHandler* + template + static AuthClientHandler* create(CephContext *cct, int proto, RotatingKeyRing *rkeys); protected: virtual void validate_tickets() = 0; diff --git a/src/auth/cephx/CephxClientHandler.cc b/src/auth/cephx/CephxClientHandler.cc index ea0b67cebbb..1748fa27e54 100644 --- a/src/auth/cephx/CephxClientHandler.cc +++ b/src/auth/cephx/CephxClientHandler.cc @@ -32,8 +32,6 @@ int CephxClientHandler::build_request(bufferlist& bl) const { ldout(cct, 10) << "build_request" << dendl; - std::shared_lock l{lock}; - if (need & CEPH_ENTITY_TYPE_AUTH) { /* authenticate */ CephXRequestHeader header; @@ -112,7 +110,6 @@ template int CephxClientHandler::handle_response(int ret, bufferlist::const_iterator& indata) { ldout(cct, 10) << "handle_response ret = " << ret << dendl; - std::unique_lock l{lock}; if (ret < 0) return ret; // hrm! @@ -206,7 +203,6 @@ int CephxClientHandler::handle_response(int ret, bufferlist::const_iterator& template AuthAuthorizer *CephxClientHandler::build_authorizer(uint32_t service_id) const { - std::shared_lock l{lock}; ldout(cct, 10) << "build_authorizer for service " << ceph_entity_type_name(service_id) << dendl; return tickets.build_authorizer(service_id); } @@ -225,7 +221,6 @@ bool CephxClientHandler::build_rotating_request(bufferlist& bl) const template void CephxClientHandler::prepare_build_request() { - std::unique_lock l{lock}; ldout(cct, 10) << "validate_tickets: want=" << want << " need=" << need << " have=" << have << dendl; validate_tickets(); @@ -245,7 +240,6 @@ void CephxClientHandler::validate_tickets() template bool CephxClientHandler::need_tickets() { - std::unique_lock l{lock}; validate_tickets(); ldout(cct, 20) << "need_tickets: want=" << want diff --git a/src/auth/cephx/CephxClientHandler.h b/src/auth/cephx/CephxClientHandler.h index eb6055c739e..7a91e0322c0 100644 --- a/src/auth/cephx/CephxClientHandler.h +++ b/src/auth/cephx/CephxClientHandler.h @@ -23,7 +23,7 @@ class CephContext; class KeyRing; template -class CephxClientHandler : public AuthClientHandler { +class CephxClientHandler : public AuthClientHandler { bool starting; /* envelope protocol parameters */ @@ -35,17 +35,10 @@ class CephxClientHandler : public AuthClientHandler { RotatingKeyRing *rotating_secrets; KeyRing *keyring; - using AuthClientHandler::cct; - using AuthClientHandler::global_id; - using AuthClientHandler::want; - using AuthClientHandler::have; - using AuthClientHandler::need; - using AuthClientHandler::lock; - public: CephxClientHandler(CephContext *cct_, RotatingKeyRing *rsecrets) - : AuthClientHandler(cct_), + : AuthClientHandler(cct_), starting(false), server_challenge(0), tickets(cct_), @@ -57,7 +50,6 @@ public: } void reset() override { - std::unique_lock l{lock}; starting = true; server_challenge = 0; } @@ -73,7 +65,6 @@ public: bool need_tickets() override; void set_global_id(uint64_t id) override { - std::unique_lock l{lock}; global_id = id; tickets.global_id = id; } diff --git a/src/auth/none/AuthNoneClientHandler.h b/src/auth/none/AuthNoneClientHandler.h index 32a410ab76d..c74acb6ec38 100644 --- a/src/auth/none/AuthNoneClientHandler.h +++ b/src/auth/none/AuthNoneClientHandler.h @@ -20,16 +20,11 @@ #include "common/ceph_context.h" #include "common/config.h" -template -class AuthNoneClientHandler : public AuthClientHandler { - using AuthClientHandler::cct; - using AuthClientHandler::global_id; - using AuthClientHandler::lock; +class AuthNoneClientHandler : public AuthClientHandler { public: - AuthNoneClientHandler(CephContext *cct_, - RotatingKeyRing *rkeys) - : AuthClientHandler(cct_) {} + AuthNoneClientHandler(CephContext *cct_) + : AuthClientHandler(cct_) {} void reset() override { } @@ -41,7 +36,6 @@ public: int get_protocol() const override { return CEPH_AUTH_NONE; } AuthAuthorizer *build_authorizer(uint32_t service_id) const override { - std::shared_lock l{lock}; AuthNoneAuthorizer *auth = new AuthNoneAuthorizer(); if (auth) { auth->build_authorizer(cct->_conf->name, global_id); @@ -52,7 +46,6 @@ public: bool need_tickets() override { return false; } void set_global_id(uint64_t id) override { - std::unique_lock l{lock}; global_id = id; } private: diff --git a/src/mon/MonClient.cc b/src/mon/MonClient.cc index 09a86749fae..56baab23858 100644 --- a/src/mon/MonClient.cc +++ b/src/mon/MonClient.cc @@ -1278,7 +1278,7 @@ int MonConnection::_negotiate(MAuthReply *m, } auth.reset( - AuthClientHandler::create(cct,m->protocol, keyring)); + AuthClientHandler::create(cct,m->protocol, keyring)); if (!auth) { ldout(cct, 10) << "no handler for protocol " << m->protocol << dendl; if (m->result == -ENOTSUP) { diff --git a/src/mon/MonClient.h b/src/mon/MonClient.h index bffd2d916e6..e359f96bd37 100644 --- a/src/mon/MonClient.h +++ b/src/mon/MonClient.h @@ -34,7 +34,7 @@ class MAuthRotating; class LogClient; class AuthAuthorizer; class AuthMethodList; -template class AuthClientHandler; +class AuthClientHandler; class KeyRing; template class RotatingKeyRing; @@ -118,7 +118,7 @@ public: ConnectionRef get_con() { return con; } - std::unique_ptr>& get_auth() { + std::unique_ptr& get_auth() { return auth; } @@ -139,7 +139,7 @@ private: State state = State::NONE; ConnectionRef con; - std::unique_ptr> auth; + std::unique_ptr auth; uint64_t global_id; }; @@ -192,7 +192,7 @@ private: bool got_config = false; // authenticate - std::unique_ptr> auth; + std::unique_ptr auth; uint32_t want_keys = 0; uint64_t global_id = 0; Cond auth_cond; diff --git a/src/mon/Monitor.cc b/src/mon/Monitor.cc index 8da36176c0a..4771c9f549c 100644 --- a/src/mon/Monitor.cc +++ b/src/mon/Monitor.cc @@ -5713,7 +5713,7 @@ bool Monitor::ms_get_authorizer(int service_id, AuthAuthorizer **authorizer, if (!auth_cluster_required.is_supported_auth(CEPH_AUTH_CEPHX)) { // auth_none dout(20) << __func__ << " building auth_none authorizer" << dendl; - AuthNoneClientHandler handler(g_ceph_context, nullptr); + AuthNoneClientHandler handler{g_ceph_context}; handler.set_global_id(0); *authorizer = handler.build_authorizer(service_id); return true; -- 2.39.5