diff --git a/src/mongo/db/auth/authorization_manager.cpp b/src/mongo/db/auth/authorization_manager.cpp index 51e0f4e03a9fb..162fb2f77c01a 100644 --- a/src/mongo/db/auth/authorization_manager.cpp +++ b/src/mongo/db/auth/authorization_manager.cpp @@ -203,6 +203,11 @@ namespace mongo { Status AuthorizationManager::acquireUser(const UserName& userName, User** acquiredUser) { boost::lock_guard lk(_lock); + return _acquireUser_inlock(userName, acquiredUser); + } + + Status AuthorizationManager::_acquireUser_inlock(const UserName& userName, + User** acquiredUser) { unordered_map::iterator it = _userCache.find(userName); if (it != _userCache.end()) { fassert(16914, it->second); @@ -410,35 +415,51 @@ namespace mongo { return Status::OK(); } - namespace { - class AuthzUpgradeLockGuard { - MONGO_DISALLOW_COPYING(AuthzUpgradeLockGuard); - public: - explicit AuthzUpgradeLockGuard(AuthzManagerExternalState* externalState) - : _externalState(externalState), _locked(false) { - } - ~AuthzUpgradeLockGuard() { - if (_locked) - unlock(); - } + AuthorizationManager::Guard::Guard(AuthorizationManager* authzManager) + : _authzManager(authzManager), + _lockedForUpdate(false), + _authzManagerLock(authzManager->_lock) {} - bool tryLock() { - fassert(17111, !_locked); - _locked = _externalState->tryLockUpgradeProcess(); - return _locked; + AuthorizationManager::Guard::~Guard() { + if (_lockedForUpdate) { + if (!_authzManagerLock.owns_lock()) { + _authzManagerLock.lock(); } + releaseAuthzUpdateLock(); + } + } - void unlock() { - fassert(17112, _locked); - _externalState->unlockUpgradeProcess(); - _locked = false; - } - private: - AuthzManagerExternalState* _externalState; - bool _locked; - }; + bool AuthorizationManager::Guard::tryAcquireAuthzUpdateLock() { + fassert(17111, !_lockedForUpdate); + fassert(17126, _authzManagerLock.owns_lock()); + _lockedForUpdate = _authzManager->_externalState->tryAcquireAuthzUpdateLock(); + return _lockedForUpdate; + } + void AuthorizationManager::Guard::releaseAuthzUpdateLock() { + fassert(17112, _lockedForUpdate); + fassert(17127, _authzManagerLock.owns_lock()); + _authzManager->_externalState->releaseAuthzUpdateLock(); + _lockedForUpdate = false; + } + + void AuthorizationManager::Guard::acquireAuthorizationManagerLock() { + fassert(17129, !_authzManagerLock.owns_lock()); + _authzManagerLock.lock(); + } + + void AuthorizationManager::Guard::releaseAuthorizationManagerLock() { + fassert(17128, _authzManagerLock.owns_lock()); + _authzManagerLock.unlock(); + } + + Status AuthorizationManager::Guard::acquireUser(const UserName& userName, User** acquiredUser) { + fassert(17130, _authzManagerLock.owns_lock()); + return _authzManager->_acquireUser_inlock(userName, acquiredUser); + } + + namespace { BSONObj userAsV2PrivilegeDocument(const User& user) { BSONObjBuilder builder; @@ -495,9 +516,8 @@ namespace mongo { } // namespace Status AuthorizationManager::upgradeAuthCollections() { - boost::lock_guard lkLocal(_lock); - AuthzUpgradeLockGuard lkUpgrade(_externalState.get()); - if (!lkUpgrade.tryLock()) { + AuthorizationManager::Guard lkUpgrade(this); + if (!lkUpgrade.tryAcquireAuthzUpdateLock()) { return Status(ErrorCodes::LockBusy, "Could not lock auth data upgrade process lock."); } int durableVersion = 0; diff --git a/src/mongo/db/auth/authorization_manager.h b/src/mongo/db/auth/authorization_manager.h index 65d94ad503b58..27063eed88a2f 100644 --- a/src/mongo/db/auth/authorization_manager.h +++ b/src/mongo/db/auth/authorization_manager.h @@ -63,6 +63,9 @@ namespace mongo { MONGO_DISALLOW_COPYING(AuthorizationManager); public: + // Locks the AuthorizationManager and guards access to its members + class Guard; + // The newly constructed AuthorizationManager takes ownership of "externalState" explicit AuthorizationManager(AuthzManagerExternalState* externalState); @@ -223,6 +226,8 @@ namespace mongo { private: + Status _acquireUser_inlock(const UserName& userName, User** acquiredUser); + /** * Returns the current version number of the authorization system. Should only be called * when holding _lock. @@ -292,6 +297,81 @@ namespace mongo { * Protects _userCache, _roleGraph, _version, and _parser. */ boost::mutex _lock; + + friend class AuthorizationManager::Guard; + }; + + + /* + * Guard object for locking an AuthorizationManager. + * There are two different locks that this object interacts with: the AuthorizationManager's + * _lock (henceforth called the AM::_lock), and the authzUpdateLock. The AM::_lock protects + * reading and writing the in-memory data structures managed by the AuthorizationManager. The + * authzUpdateLock is the lock that serializes all writes to the persistent authorization + * documents (admin.system.users, admin.system.roles, admin.system.version). + * When the guard is constructed it initially locks the AM::_lock. The guard's destructor will + * always release the AM::_lock if it is held. + * The guard then provides some public methods that allow interaction with the + * AuthorizationManager while inside the AM::_lock. + * If modifications to the authorization documents in persistent storage is required, then + * consumers of the guard can call tryAcquireAuthzUpdateLock() to lock the authzUpdateLock. + * If that is successful, consumers can then call releaseAuthorizationManagerLock to unlock the + * AM::_lock, while keeping the authzUpdateLock locked. This allows + * modifications to the authorization documents to occur without the AM::_lock needing to be + * held the whole time (which prevents authentication and access control + * checks). + * Since changing the state of the authzUpdateLock requires the AM::_lock to + * be held, if the guard's destructor is called when the authzUpdateLock is held but the + * AM::_lock is not, it will first acquire the AM::_lock, then release the authzUpdateLock, + * then finally release the AM::_lock. + * + * Note: This locking semantics only works because we never block to acquire the + * authzUpdateLock. If a blocking acquireAuthzUpdateLock were introduced, it could introduce + * deadlocks. + */ + class AuthorizationManager::Guard { + MONGO_DISALLOW_COPYING(Guard); + public: + explicit Guard(AuthorizationManager* authzManager); + ~Guard(); + + /** + * Tries to acquire the global lock guarding modifications to all persistent data related + * to authorization, namely the admin.system.users, admin.system.roles, and + * admin.system.version collections. This serializes all writers to the authorization + * documents, but does not impact readers. + * The AuthorizationManager's _lock must be held before this is called. + */ + bool tryAcquireAuthzUpdateLock(); + + /** + * Releases the lock guarding modifications to persistent authorization data, which must + * already be held. + * The AuthorizationManager's _lock must be held before this is called. + */ + void releaseAuthzUpdateLock(); + + /** + * Releases the AuthorizationManager's _lock. + */ + void releaseAuthorizationManagerLock(); + + /** + * Acquires the AuthorizationManager's _lock. + */ + void acquireAuthorizationManagerLock(); + + /** + * Delegates to AuthorizationManager's _acquireUser_inlock method. + */ + Status acquireUser(const UserName& userName, User** acquiredUser); + + private: + AuthorizationManager* _authzManager; + // True if the Guard has locked the lock that guards modifications to authz documents. + bool _lockedForUpdate; + // For locking the AuthorizationManager's _lock + boost::unique_lock _authzManagerLock; }; } // namespace mongo diff --git a/src/mongo/db/auth/authz_manager_external_state.h b/src/mongo/db/auth/authz_manager_external_state.h index fd3c3e0b3bc39..19fa865abe274 100644 --- a/src/mongo/db/auth/authz_manager_external_state.h +++ b/src/mongo/db/auth/authz_manager_external_state.h @@ -150,14 +150,18 @@ namespace mongo { const NamespaceString& toName) = 0; /** - * Tries to acquire the global lock guarding the upgrade process. + * Tries to acquire the global lock guarding modifications to all persistent data related + * to authorization, namely the admin.system.users, admin.system.roles, and + * admin.system.version collections. This serializes all writers to the authorization + * documents, but does not impact readers. */ - virtual bool tryLockUpgradeProcess() = 0; + virtual bool tryAcquireAuthzUpdateLock() = 0; /** - * Releases the lock guarding the upgrade process, which must already be held. + * Releases the lock guarding modifications to persistent authorization data, which must + * already be held. */ - virtual void unlockUpgradeProcess() = 0; + virtual void releaseAuthzUpdateLock() = 0; protected: AuthzManagerExternalState(); // This class should never be instantiated directly. diff --git a/src/mongo/db/auth/authz_manager_external_state_d.cpp b/src/mongo/db/auth/authz_manager_external_state_d.cpp index 91749a480543d..72e01e5f29e47 100644 --- a/src/mongo/db/auth/authz_manager_external_state_d.cpp +++ b/src/mongo/db/auth/authz_manager_external_state_d.cpp @@ -231,11 +231,11 @@ namespace { fassertFailed(17098); } - bool AuthzManagerExternalStateMongod::tryLockUpgradeProcess() { + bool AuthzManagerExternalStateMongod::tryAcquireAuthzUpdateLock() { fassertFailed(17099); } - void AuthzManagerExternalStateMongod::unlockUpgradeProcess() { + void AuthzManagerExternalStateMongod::releaseAuthzUpdateLock() { fassertFailed(17100); } diff --git a/src/mongo/db/auth/authz_manager_external_state_d.h b/src/mongo/db/auth/authz_manager_external_state_d.h index 8d9bfc0fa56c4..a9c050be5cb62 100644 --- a/src/mongo/db/auth/authz_manager_external_state_d.h +++ b/src/mongo/db/auth/authz_manager_external_state_d.h @@ -79,8 +79,8 @@ namespace mongo { const NamespaceString& newName); virtual Status copyCollection(const NamespaceString& fromName, const NamespaceString& toName); - virtual bool tryLockUpgradeProcess(); - virtual void unlockUpgradeProcess(); + virtual bool tryAcquireAuthzUpdateLock(); + virtual void releaseAuthzUpdateLock(); protected: virtual Status _findUser(const string& usersNamespace, diff --git a/src/mongo/db/auth/authz_manager_external_state_mock.cpp b/src/mongo/db/auth/authz_manager_external_state_mock.cpp index dd9bfb3a9aa08..546c36387f213 100644 --- a/src/mongo/db/auth/authz_manager_external_state_mock.cpp +++ b/src/mongo/db/auth/authz_manager_external_state_mock.cpp @@ -208,11 +208,11 @@ namespace mongo { return Status::OK(); } - bool AuthzManagerExternalStateMock::tryLockUpgradeProcess() { + bool AuthzManagerExternalStateMock::tryAcquireAuthzUpdateLock() { return true; } - void AuthzManagerExternalStateMock::unlockUpgradeProcess() {} + void AuthzManagerExternalStateMock::releaseAuthzUpdateLock() {} std::vector AuthzManagerExternalStateMock::getCollectionContents( const NamespaceString& collectionName) { diff --git a/src/mongo/db/auth/authz_manager_external_state_mock.h b/src/mongo/db/auth/authz_manager_external_state_mock.h index 2bb03e0a28608..fbfa8531ddeee 100644 --- a/src/mongo/db/auth/authz_manager_external_state_mock.h +++ b/src/mongo/db/auth/authz_manager_external_state_mock.h @@ -94,8 +94,8 @@ namespace mongo { const NamespaceString& newName); virtual Status copyCollection(const NamespaceString& fromName, const NamespaceString& toName); - virtual bool tryLockUpgradeProcess(); - virtual void unlockUpgradeProcess(); + virtual bool tryAcquireAuthzUpdateLock(); + virtual void releaseAuthzUpdateLock(); std::vector getCollectionContents(const NamespaceString& collectionName); diff --git a/src/mongo/db/auth/authz_manager_external_state_s.cpp b/src/mongo/db/auth/authz_manager_external_state_s.cpp index 87979a4e6436f..79bda6f4f8b48 100644 --- a/src/mongo/db/auth/authz_manager_external_state_s.cpp +++ b/src/mongo/db/auth/authz_manager_external_state_s.cpp @@ -258,11 +258,11 @@ namespace { fassertFailed(17108); } - bool AuthzManagerExternalStateMongos::tryLockUpgradeProcess() { + bool AuthzManagerExternalStateMongos::tryAcquireAuthzUpdateLock() { fassertFailed(17109); } - void AuthzManagerExternalStateMongos::unlockUpgradeProcess() { + void AuthzManagerExternalStateMongos::releaseAuthzUpdateLock() { fassertFailed(17110); } diff --git a/src/mongo/db/auth/authz_manager_external_state_s.h b/src/mongo/db/auth/authz_manager_external_state_s.h index 0a8a39d64ffb5..903143e707f0f 100644 --- a/src/mongo/db/auth/authz_manager_external_state_s.h +++ b/src/mongo/db/auth/authz_manager_external_state_s.h @@ -79,8 +79,8 @@ namespace mongo { const NamespaceString& newName); virtual Status copyCollection(const NamespaceString& fromName, const NamespaceString& toName); - virtual bool tryLockUpgradeProcess(); - virtual void unlockUpgradeProcess(); + virtual bool tryAcquireAuthzUpdateLock(); + virtual void releaseAuthzUpdateLock(); protected: virtual Status _findUser(const string& usersNamespace,