From c2405957edc3f837ee92a661557f36555d6b1785 Mon Sep 17 00:00:00 2001 From: Patrick Donnelly Date: Wed, 30 Jan 2019 15:52:06 -0800 Subject: [PATCH] mds: move session setup to ms_handle_accept Session setup in ms_handle_authentication is (historically) racy where multiple connections from the same client can come in before one is finally accepted. A session should only be created after ms_handle_accept. The MDS did some backflips before this commit to ensure this. Moreover, with the msgr2 changes, it is even more necessary since the address nonce is not set until before ms_handle_accept is called. Signed-off-by: Patrick Donnelly --- src/mds/MDSAuthCaps.h | 18 +++++--- src/mds/MDSDaemon.cc | 103 ++++++++++++++++++------------------------ src/mds/MDSDaemon.h | 2 + src/msg/Messenger.cc | 2 +- 4 files changed, 57 insertions(+), 68 deletions(-) diff --git a/src/mds/MDSAuthCaps.h b/src/mds/MDSAuthCaps.h index c21df082137..cc1006cdf66 100644 --- a/src/mds/MDSAuthCaps.h +++ b/src/mds/MDSAuthCaps.h @@ -52,7 +52,8 @@ struct MDSCapSpec { static const unsigned RWS = (READ|WRITE|SNAPSHOT); static const unsigned RWPS = (READ|WRITE|SET_VXATTR|SNAPSHOT); - MDSCapSpec(unsigned _caps=0) : caps(_caps) { + MDSCapSpec() = default; + MDSCapSpec(unsigned _caps) : caps(_caps) { if (caps & ALL) caps |= RWPS; } @@ -84,7 +85,7 @@ struct MDSCapSpec { return (caps & SET_VXATTR); } private: - unsigned caps; + unsigned caps = 0; }; // conditions before we are allowed to do it @@ -153,16 +154,19 @@ struct MDSCapGrant { class MDSAuthCaps { - CephContext *cct; + CephContext *cct = nullptr; std::vector grants; public: - explicit MDSAuthCaps(CephContext *cct_=NULL) - : cct(cct_) { } + MDSAuthCaps() = default; + explicit MDSAuthCaps(CephContext *cct_) : cct(cct_) {} // this ctor is used by spirit/phoenix; doesn't need cct. - explicit MDSAuthCaps(const std::vector &grants_) - : cct(NULL), grants(grants_) { } + explicit MDSAuthCaps(const std::vector& grants_) : grants(grants_) {} + + void clear() { + grants.clear(); + } void set_allow_all(); bool parse(CephContext *cct, std::string_view str, std::ostream *err); diff --git a/src/mds/MDSDaemon.cc b/src/mds/MDSDaemon.cc index 0c77f0d70bc..2ce3784e6c5 100644 --- a/src/mds/MDSDaemon.cc +++ b/src/mds/MDSDaemon.cc @@ -1230,7 +1230,7 @@ bool MDSDaemon::ms_handle_reset(Connection *con) if (stopping) { return false; } - dout(5) << "ms_handle_reset on " << con->get_peer_addr() << dendl; + dout(5) << "ms_handle_reset on " << con->get_peer_socket_addr() << dendl; if (beacon.get_want_state() == CEPH_MDS_STATE_DNE) return false; @@ -1258,7 +1258,7 @@ void MDSDaemon::ms_handle_remote_reset(Connection *con) return; } - dout(5) << "ms_handle_remote_reset on " << con->get_peer_addr() << dendl; + dout(5) << "ms_handle_remote_reset on " << con->get_peer_socket_addr() << dendl; if (beacon.get_want_state() == CEPH_MDS_STATE_DNE) return; @@ -1283,11 +1283,47 @@ KeyStore *MDSDaemon::ms_get_auth1_authorizer_keystore() return monc->rotating_secrets.get(); } +bool MDSDaemon::parse_caps(const AuthCapsInfo& info, MDSAuthCaps& caps) +{ + caps.clear(); + if (info.allow_all) { + caps.set_allow_all(); + return true; + } else { + auto it = info.caps.begin(); + string auth_cap_str; + try { + decode(auth_cap_str, it); + } catch (const buffer::error& e) { + dout(1) << __func__ << ": cannot decode auth caps buffer of length " << info.caps.length() << dendl; + return false; + } + + dout(10) << __func__ << ": parsing auth_cap_str='" << auth_cap_str << "'" << dendl; + CachedStackStringStream cs; + if (caps.parse(g_ceph_context, auth_cap_str, cs.get())) { + return true; + } else { + dout(1) << __func__ << ": auth cap parse error: " << cs->strv() << " parsing '" << auth_cap_str << "'" << dendl; + return false; + } + } +} + int MDSDaemon::ms_handle_authentication(Connection *con) { - std::lock_guard l(mds_lock); - int ret = 0; + /* N.B. without mds_lock! */ + MDSAuthCaps caps; + return parse_caps(con->get_peer_caps_info(), caps) ? 0 : -1; +} + +void MDSDaemon::ms_handle_accept(Connection *con) +{ entity_name_t n(con->get_peer_type(), con->get_peer_global_id()); + std::lock_guard l(mds_lock); + if (stopping) { + return; + } // We allow connections and assign Session instances to connections // even if we have not been assigned a rank, because clients with @@ -1306,7 +1342,7 @@ int MDSDaemon::ms_handle_authentication(Connection *con) if (!s) { s = new Session(con); s->info.auth_name = con->get_peer_entity_name(); - s->info.inst.addr = con->get_peer_addr(); + s->info.inst.addr = con->get_peer_socket_addr(); s->info.inst.name = n; dout(10) << " new session " << s << " for " << s->info.inst << " con " << con << dendl; @@ -1319,64 +1355,11 @@ int MDSDaemon::ms_handle_authentication(Connection *con) << " existing con " << s->get_connection() << ", new/authorizing con " << con << dendl; con->set_priv(RefCountedPtr{s}); - - // Wait until we fully accept the connection before setting - // s->connection. In particular, if there are multiple incoming - // connection attempts, they will all get their authorizer - // validated, but some of them may "lose the race" and get - // dropped. We only want to consider the winner(s). See - // ms_handle_accept(). This is important for Sessions we replay - // from the journal on recovery that don't have established - // messenger state; we want the con from only the winning - // connect attempt(s). (Normal reconnects that don't follow MDS - // recovery are reconnected to the existing con by the - // messenger.) - } - - AuthCapsInfo &caps_info = con->get_peer_caps_info(); - if (caps_info.allow_all) { - // Flag for auth providers that don't provide cap strings - s->auth_caps.set_allow_all(); - } else { - auto p = caps_info.caps.cbegin(); - string auth_cap_str; - try { - decode(auth_cap_str, p); - - dout(10) << __func__ << ": parsing auth_cap_str='" << auth_cap_str << "'" - << dendl; - std::ostringstream errstr; - if (!s->auth_caps.parse(g_ceph_context, auth_cap_str, &errstr)) { - dout(1) << __func__ << ": auth cap parse error: " << errstr.str() - << " parsing '" << auth_cap_str << "'" << dendl; - clog->warn() << name << " mds cap '" << auth_cap_str - << "' does not parse: " << errstr.str(); - ret = -EPERM; - } else { - ret = 1; - } - } catch (buffer::error& e) { - // Assume legacy auth, defaults to: - // * permit all filesystem ops - // * permit no `tell` ops - dout(1) << __func__ << ": cannot decode auth caps bl of length " - << caps_info.caps.length() << dendl; - ret = -EPERM; - } } - return ret; -} -void MDSDaemon::ms_handle_accept(Connection *con) -{ - std::lock_guard l(mds_lock); - if (stopping) { - return; - } + parse_caps(con->get_peer_caps_info(), s->auth_caps); - auto priv = con->get_priv(); - auto s = static_cast(priv.get()); - dout(10) << "ms_handle_accept " << con->get_peer_addr() << " con " << con << " session " << s << dendl; + dout(10) << "ms_handle_accept " << con->get_peer_socket_addr() << " con " << con << " session " << s << dendl; if (s) { if (s->get_connection() != con) { dout(10) << " session connection " << s->get_connection() diff --git a/src/mds/MDSDaemon.h b/src/mds/MDSDaemon.h index 130f1044ccf..5cdcd5fa514 100644 --- a/src/mds/MDSDaemon.h +++ b/src/mds/MDSDaemon.h @@ -178,6 +178,8 @@ private: static const std::vector& get_commands(); + bool parse_caps(const AuthCapsInfo&, MDSAuthCaps&); + mono_time starttime = mono_clock::zero(); }; diff --git a/src/msg/Messenger.cc b/src/msg/Messenger.cc index 7569acbb341..51c1c76b0ec 100644 --- a/src/msg/Messenger.cc +++ b/src/msg/Messenger.cc @@ -207,7 +207,7 @@ bool Messenger::ms_deliver_verify_authorizer( connection_secret, challenge); if (isvalid) { - dis->ms_handle_authentication(con); + return dis->ms_handle_authentication(con)>=0; } return true; } -- 2.39.5