From 1672364497975ea22d1a14f1094ca8bb582d565c Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 4 Dec 2023 19:21:41 -0500 Subject: [PATCH] Fix session eviction handling of eviction hints. (#30806) When we establish a session as a CASE responder, we try to allocate a new session to listen to session establishments, and use the just-established-session's peer ID as the eviction hint. If we had a bunch of active sessions on a fabric, all to different nodes, this would cause the just-established session to be evicted, since it matched the hint. The fix is to only consider sessions for eviction based on the hint if they are either non-active or not unique sessions to the peer (at which point, the just-established session should be last in priority order for eviction). Fixes https://github.com/project-chip/connectedhomeip/issues/30728 --- src/transport/SecureSessionTable.cpp | 27 ++++- src/transport/SecureSessionTable.h | 2 +- .../tests/TestSecureSessionTable.cpp | 107 +++++++++++++++++- 3 files changed, 127 insertions(+), 9 deletions(-) diff --git a/src/transport/SecureSessionTable.cpp b/src/transport/SecureSessionTable.cpp index aa98637345e9e5..bc201b33007094 100644 --- a/src/transport/SecureSessionTable.cpp +++ b/src/transport/SecureSessionTable.cpp @@ -265,8 +265,31 @@ void SecureSessionTable::DefaultEvictionPolicy(EvictionPolicyContext & evictionC return a.mNumMatchingOnPeer > b.mNumMatchingOnPeer; } - int doesAMatchSessionHint = a.mSession->GetPeer() == evictionContext.GetSessionEvictionHint(); - int doesBMatchSessionHint = b.mSession->GetPeer() == evictionContext.GetSessionEvictionHint(); + // We have an evicton hint in two cases: + // + // 1) When we just established CASE as a responder, the hint is the node + // we just established CASE to. + // 2) When starting to establish CASE as an initiator, the hint is the + // node we are going to establish CASE to. + // + // In case 2, we should not end up here if there is an active session to + // the peer at all (because that session should have been used instead + // of establishing a new one). + // + // In case 1, we know we have a session matching the hint, but we don't + // want to pick that one for eviction, because we just established it. + // So we should not consider a session as matching a hint if it's active + // and is the only session to our peer. + // + // Checking for the "active" state in addition to the "only session to + // peer" state allows us to prioritize evicting defuct sessions that + // match the hint against other defunct sessions. + auto sessionMatchesEvictionHint = [&evictionContext](const SortableSession & session) -> int { + return session.mSession->GetPeer() == evictionContext.GetSessionEvictionHint() && + (!session.mSession->IsActiveSession() || session.mNumMatchingOnPeer > 0); + }; + int doesAMatchSessionHint = sessionMatchesEvictionHint(a); + int doesBMatchSessionHint = sessionMatchesEvictionHint(b); // // Sorting on Key4 diff --git a/src/transport/SecureSessionTable.h b/src/transport/SecureSessionTable.h index aff58c4e5fe0b0..7f8a072a03a2fe 100644 --- a/src/transport/SecureSessionTable.h +++ b/src/transport/SecureSessionTable.h @@ -206,7 +206,7 @@ class SecureSessionTable * the session that is most ahead as the best candidate for eviction: * * - Key1: Sessions on fabrics that have more sessions in the table are placed ahead of sessions on fabrics - * with lesser sessions. We conclusively know that if a particular fabric has more sessions in the table + * with fewer sessions. We conclusively know that if a particular fabric has more sessions in the table * than another, then that fabric is definitely over minimas (assuming a minimally sized session table * conformant to spec minimas). * diff --git a/src/transport/tests/TestSecureSessionTable.cpp b/src/transport/tests/TestSecureSessionTable.cpp index 980e07e955015f..7dbbeba654117f 100644 --- a/src/transport/tests/TestSecureSessionTable.cpp +++ b/src/transport/tests/TestSecureSessionTable.cpp @@ -120,10 +120,14 @@ void TestSecureSessionTable::CreateSessionTable(std::vector & ScopedNodeId(1, sessionParams[i].mPeer.GetFabricIndex()), sessionParams[i].mPeer, CATValues(), static_cast(i), ReliableMessageProtocolConfig(System::Clock::Milliseconds32(0), System::Clock::Milliseconds32(0), System::Clock::Milliseconds16(0))); - session.Value()->AsSecureSession()->mLastActivityTime = sessionParams[i].mLastActivityTime; - session.Value()->AsSecureSession()->mState = sessionParams[i].mState; + // Make sure we set up our holder _before_ the session goes into a state + // other than active, because holders refuse to hold non-active + // sessions. mSessionList.push_back(Platform::MakeUnique(session.Value())); + + session.Value()->AsSecureSession()->mLastActivityTime = sessionParams[i].mLastActivityTime; + session.Value()->AsSecureSession()->mState = sessionParams[i].mState; } } @@ -279,12 +283,44 @@ void TestSecureSessionTable::ValidateSessionSorting(nlTestSuite * inSuite, void // This validates evicting from a table with equally loaded fabrics. In this scenario, // bias is given to the fabric that matches that of the eviction hint. // - // There are equal sessions to Node 2 as well as Node 3 in that fabric, so the Node - // that matches the session eviction hint will be selected, and in that, the older session. + // There is an equal number sessions to nodes 1, 2, and 3 in that fabric, so the Node + // that matches the session eviction hint will be selected. + // + // All the sessions in the table are defunct, because for unique active + // sessions eviction hints are ignored. // { - ChipLogProgress(SecureChannel, - "-------- Equal Fabrics Eviction (Equal # Sessions to Nodes, Hint Match On Fabric & Node) ---------"); + ChipLogProgress( + SecureChannel, + "-------- Equal Fabrics Eviction (Single equal # Sessions to Nodes, Hint Match On Fabric & Node) ---------"); + + std::vector sessionParamList = { + { { 1, kFabric1 }, System::Clock::Timestamp(9), SecureSession::State::kDefunct }, + { { 1, kFabric2 }, System::Clock::Timestamp(3), SecureSession::State::kDefunct }, + { { 2, kFabric1 }, System::Clock::Timestamp(2), SecureSession::State::kDefunct }, + { { 3, kFabric1 }, System::Clock::Timestamp(7), SecureSession::State::kDefunct }, + { { 3, kFabric2 }, System::Clock::Timestamp(1), SecureSession::State::kDefunct }, + { { 4, kFabric2 }, System::Clock::Timestamp(2), SecureSession::State::kDefunct }, + }; + + _this->CreateSessionTable(sessionParamList); + _this->AllocateSession(ScopedNodeId(3, kFabric1), sessionParamList, 3); + } + + // + // This validates evicting from a table with equally loaded fabrics. In this scenario, + // bias is given to the fabric that matches that of the eviction hint. + // + // There is an equal number sessions to nodes 1, 2, and 3 in that fabric, so the Node + // that matches the session eviction hint will be selected. + // + // All the peers in this table have two sessions to them, so that we pay + // attention to the eviction hint. The older of the two should be selected. + // + { + ChipLogProgress( + SecureChannel, + "-------- Equal Fabrics Eviction (Multiple equal # Sessions to Nodes, Hint Match On Fabric & Node) ---------"); std::vector sessionParamList = { { { 1, kFabric1 }, System::Clock::Timestamp(9), SecureSession::State::kActive }, @@ -293,12 +329,71 @@ void TestSecureSessionTable::ValidateSessionSorting(nlTestSuite * inSuite, void { { 3, kFabric1 }, System::Clock::Timestamp(7), SecureSession::State::kActive }, { { 3, kFabric2 }, System::Clock::Timestamp(1), SecureSession::State::kActive }, { { 4, kFabric2 }, System::Clock::Timestamp(2), SecureSession::State::kActive }, + { { 1, kFabric1 }, System::Clock::Timestamp(10), SecureSession::State::kActive }, + { { 1, kFabric2 }, System::Clock::Timestamp(4), SecureSession::State::kActive }, + { { 2, kFabric1 }, System::Clock::Timestamp(3), SecureSession::State::kActive }, + { { 3, kFabric1 }, System::Clock::Timestamp(8), SecureSession::State::kActive }, + { { 3, kFabric2 }, System::Clock::Timestamp(2), SecureSession::State::kActive }, + { { 4, kFabric2 }, System::Clock::Timestamp(3), SecureSession::State::kActive }, }; _this->CreateSessionTable(sessionParamList); _this->AllocateSession(ScopedNodeId(3, kFabric1), sessionParamList, 3); } + // + // This validates evicting from a table with equally loaded fabrics. In this scenario, + // bias is given to the fabric that matches that of the eviction hint. + // + // There is an equal sessions to nodes 1, 2, and 3 in that fabric, and only + // one per node. Since all the sessions are active, the eviction hint's + // node id will be ignored and the oldest session on the fabric will be selected. + // + { + ChipLogProgress(SecureChannel, + "-------- Equal Fabrics Eviction (Equal # Sessions to Nodes, Hint Match On Fabric & Node, hint node " + "ignored) ---------"); + + std::vector sessionParamList = { + { { 1, kFabric1 }, System::Clock::Timestamp(9), SecureSession::State::kActive }, + { { 1, kFabric2 }, System::Clock::Timestamp(3), SecureSession::State::kActive }, + { { 2, kFabric1 }, System::Clock::Timestamp(2), SecureSession::State::kActive }, + { { 3, kFabric1 }, System::Clock::Timestamp(7), SecureSession::State::kActive }, + { { 3, kFabric2 }, System::Clock::Timestamp(1), SecureSession::State::kActive }, + { { 4, kFabric2 }, System::Clock::Timestamp(2), SecureSession::State::kActive }, + }; + + _this->CreateSessionTable(sessionParamList); + _this->AllocateSession(ScopedNodeId(3, kFabric1), sessionParamList, 2); + } + + // + // This validates evicting from a table with equally loaded fabrics. In this scenario, + // bias is given to the fabric that matches that of the eviction hint. + // + // There is an equal sessions to nodes 1, 2, and 3 in that fabric, and only + // one per node. Since the hinted session is active, the eviction hint's + // node id will be ignored and the defunct session will be selected, even + // though it's the newest one. + // + { + ChipLogProgress(SecureChannel, + "-------- Equal Fabrics Eviction (Equal # Sessions to Nodes, Hint Match On Fabric & Node, hint node " + "ignored and state wins) ---------"); + + std::vector sessionParamList = { + { { 1, kFabric1 }, System::Clock::Timestamp(9), SecureSession::State::kDefunct }, + { { 1, kFabric2 }, System::Clock::Timestamp(3), SecureSession::State::kActive }, + { { 2, kFabric1 }, System::Clock::Timestamp(2), SecureSession::State::kActive }, + { { 3, kFabric1 }, System::Clock::Timestamp(7), SecureSession::State::kActive }, + { { 3, kFabric2 }, System::Clock::Timestamp(1), SecureSession::State::kActive }, + { { 4, kFabric2 }, System::Clock::Timestamp(2), SecureSession::State::kActive }, + }; + + _this->CreateSessionTable(sessionParamList); + _this->AllocateSession(ScopedNodeId(3, kFabric1), sessionParamList, 0); + } + // // Similar to above, except that the eviction hint matches a given fabric (kFabric1) in the // session table, but not any nodes. In this case, the oldest session in that fabric is selected