[2/3] CASE Eviction (Initial Impl)

src/app/OperationalDeviceProxy.cpp

@@ -317,12 +317,21 @@ void OperationalDeviceProxy::OnSessionEstablished(const SessionHandle & session)
 CHIP_ERROR OperationalDeviceProxy::Disconnect()
 {
     ReturnErrorCodeIf(mState != State::SecureConnected, CHIP_ERROR_INCORRECT_STATE);
+
     if (mSecureSession)
     {
-        mInitParams.sessionManager->ExpirePairing(mSecureSession.Get().Value());
+        //
+        // Mark the session as defunct to signal that we no longer want to use this
+        // session anymore for further interactions to this peer. However, if we receive
+        // messages back from that peer on the defunct session, it will bring it back into an active
+        // state again.
+        //
+        mSecureSession.Get().Value()->AsSecureSession()->MarkAsDefunct();
     }
-    MoveToState(State::HasAddress);
 
+    mSecureSession.Release();
+
+    MoveToState(State::HasAddress);
     return CHIP_NO_ERROR;
 }
 

src/controller/CommissioneeDeviceProxy.cpp

@@ -61,8 +61,9 @@ CHIP_ERROR CommissioneeDeviceProxy::CloseSession()
     ReturnErrorCodeIf(mState != ConnectionState::SecureConnected, CHIP_ERROR_INCORRECT_STATE);
     if (mSecureSession)
     {
-        mSessionManager->ExpirePairing(mSecureSession.Get().Value());
+        mSecureSession->AsSecureSession()->MarkForEviction();
     }
+
     mState = ConnectionState::NotConnected;
     mPairing.Clear();
     return CHIP_NO_ERROR;
@@ -97,6 +98,8 @@ CHIP_ERROR CommissioneeDeviceProxy::UpdateDeviceData(const Transport::PeerAddres
 CHIP_ERROR CommissioneeDeviceProxy::SetConnected(const SessionHandle & session)
 {
     VerifyOrReturnError(mState == ConnectionState::Connecting, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(session->AsSecureSession()->IsPASESession(), CHIP_ERROR_INVALID_ARGUMENT);
+
     if (!mSecureSession.Grab(session))
     {
         mState = ConnectionState::NotConnected;

src/controller/python/ChipDeviceController-StorageDelegate.cpp

@@ -126,6 +126,7 @@ CHIP_ERROR StorageAdapter::SyncDeleteKeyValue(const char * key)
         return err;
     }
 
+    ChipLogDetail(Controller, "StorageAdapter::DeleteKeyValue: Key = %s", key);
     mDeleteKeyCb(mContext, key);
     return CHIP_NO_ERROR;
 }

src/controller/python/chip/storage/__init__.py

@@ -36,7 +36,7 @@
     None, py_object, c_char_p, POINTER(c_char),  c_uint16)
 _SyncGetKeyValueCbFunct = CFUNCTYPE(
     None, py_object, c_char_p, POINTER(c_char), POINTER(c_uint16))
-_SyncDeleteKeyValueCbFunct = CFUNCTYPE(None, py_object, POINTER(c_char_p))
+_SyncDeleteKeyValueCbFunct = CFUNCTYPE(None, py_object, c_char_p)
 
 
 @_SyncSetKeyValueCbFunct

src/controller/python/test/test_scripts/base.py

@@ -178,6 +178,8 @@ def __init__(self, nodeid: int, paaTrustStorePath: str, testCommissioner: bool =
         self.logger = logger
         self.paaTrustStorePath = paaTrustStorePath
 
+        logging.getLogger().setLevel(logging.DEBUG)
+
     def _WaitForOneDiscoveredDevice(self, timeoutSeconds: int = 2):
         print("Waiting for device responses...")
         strlen = 100
@@ -356,6 +358,38 @@ def TestFailsafe(self, nodeid: int):
             return True
         return False
 
+    async def TestCaseEviction(self, nodeid: int):
+        self.logger.info("Testing CASE eviction")
+
+        minimumCASESessionsPerFabric = 3
+        minimumSupportedFabrics = 16
+
+        #
+        # This test exercises the ability to allocate more sessions than are supported in the
+        # pool configuration. By going beyond (minimumSupportedFabrics * minimumCASESessionsPerFabric),
+        # it starts to test out the eviction logic. This specific test does not validate the specifics
+        # of eviction, just that allocation and CASE session establishment proceeds successfully on both
+        # the controller and target.
+        #
+        for x in range(minimumSupportedFabrics * minimumCASESessionsPerFabric * 3):
+            self.devCtrl.CloseSession(nodeid)
+            await self.devCtrl.ReadAttribute(nodeid, [(Clusters.Basic.Attributes.ClusterRevision)])
+
+        self.logger.info("Testing CASE defunct logic")
+
+        #
+        # This tests establishing a subscription on a given CASE session, then mark it defunct (to simulate
+        # encountering a transport timeout on the session).
+        #
+        # At the max interval, we should still have a valid subscription.
+        #
+        sub = await self.devCtrl.ReadAttribute(nodeid, [(Clusters.Basic.Attributes.ClusterRevision)], reportInterval=(0, 2))
+        await asyncio.sleep(2)
+        self.devCtrl.CloseSession(nodeid)
+        await asyncio.sleep(4)
+
+        return True
+
     async def TestMultiFabric(self, ip: str, setuppin: int, nodeid: int):
         self.logger.info("Opening Commissioning Window")
 

src/controller/python/test/test_scripts/mobile-device-test.py

@@ -157,6 +157,8 @@ def do_tests(controller_nodeid, device_nodeid, address, timeout, discriminator,
     ethernet_commissioning(test, discriminator, setup_pin, address,
                            device_nodeid)
 
+    FailIfNot(asyncio.run(test.TestCaseEviction(device_nodeid)), "Failed TestCaseEviction")
+
     logger.info("Testing resolve")
     FailIfNot(test.TestResolve(nodeid=device_nodeid),
               "Failed to resolve nodeid")

src/lib/core/CHIPConfig.h

@@ -650,11 +650,16 @@
  * secure sessions. This controls the maximum number of concurrent
  * established secure sessions across all supported transports.
  *
- * This is sized to cover the sum of the following:
+ * This is sized by default to cover the sum of the following:
  *  - At least 3 CASE sessions / fabric (Spec Ref: 4.13.2.8)
  *  - 1 reserved slot for CASEServer as a responder.
  *  - 1 reserved slot for PASE.
  *
+ *  NOTE: On heap-based platforms, there is no pre-allocation of the pool.
+ *  Due to the use of an LRU-scheme to manage sessions, the actual active
+ *  size of the pool will grow up to the value of this define,
+ *  after which, it will remain at or around this size indefinitely.
+ *
  */
 #ifndef CHIP_CONFIG_SECURE_SESSION_POOL_SIZE
 #define CHIP_CONFIG_SECURE_SESSION_POOL_SIZE (CHIP_CONFIG_MAX_FABRICS * 3 + 2)

src/lib/core/CHIPPersistentStorageDelegate.h

@@ -93,7 +93,7 @@ class DLL_EXPORT PersistentStorageDelegate
      *
      * @param[in] key Key to be deleted
      *
-     * @return CHIP_NO_ERROR on success, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND the key is not found in storage,
+     * @return CHIP_NO_ERROR on success, CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if the key is not found in storage,
      *         or another CHIP_ERROR value from implementation on failure.
      */
     virtual CHIP_ERROR SyncDeleteKeyValue(const char * key) = 0;

src/lib/support/CodeUtils.h

@@ -285,12 +285,29 @@ constexpr inline const _T & max(const _T & a, const _T & b)
  *  @param[in]  expr        A Boolean expression to be evaluated.
  *  @param[in]  code        A value to return if @a expr is false.
  */
-#define VerifyOrReturnError(expr, code)                                                                                            \
+#define VerifyOrReturnError(expr, code) VerifyOrReturnValue(expr, code)
+
+/**
+ *  @def VerifyOrReturnValue(expr, value)
+ *
+ *  @brief
+ *    Returns a specified value if expression evaluates to false
+ *
+ *  Example usage:
+ *
+ *  @code
+ *    VerifyOrReturnError(param != nullptr, Foo());
+ *  @endcode
+ *
+ *  @param[in]  expr        A Boolean expression to be evaluated.
+ *  @param[in]  value       A value to return if @a expr is false.
+ */
+#define VerifyOrReturnValue(expr, value)                                                                                           \
     do                                                                                                                             \
     {                                                                                                                              \
         if (!(expr))                                                                                                               \
         {                                                                                                                          \
-            return code;                                                                                                           \
+            return (value);                                                                                                        \
         }                                                                                                                          \
     } while (false)
 

src/messaging/ExchangeMgr.cpp

@@ -240,7 +240,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
     // If it's not a duplicate message, search for an unsolicited message handler if it is marked as being sent by an initiator.
     // Since we didn't find an existing exchange that matches the message, it must be an unsolicited message. However all
     // unsolicited messages must be marked as being from an initiator.
-    if (session->IsActiveSession() && !msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsInitiator())
+    if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsInitiator())
     {
         // Search for an unsolicited message handler that can handle the message. Prefer handlers that can explicitly
         // handle the message type over handlers that handle all messages for a profile.

src/messaging/tests/MessagingContext.cpp

@@ -126,15 +126,15 @@ void MessagingContext::ExpireSessionBobToAlice()
 {
     if (mSessionBobToAlice)
     {
-        mSessionManager.ExpirePairing(mSessionBobToAlice.Get().Value());
+        mSessionBobToAlice.Get().Value()->AsSecureSession()->MarkForEviction();
     }
 }
 
 void MessagingContext::ExpireSessionAliceToBob()
 {
     if (mSessionAliceToBob)
     {
-        mSessionManager.ExpirePairing(mSessionAliceToBob.Get().Value());
+        mSessionAliceToBob.Get().Value()->AsSecureSession()->MarkForEviction();
     }
 }
 

src/messaging/tests/TestExchangeMgr.cpp

@@ -120,7 +120,7 @@ void CheckSessionExpirationBasics(nlTestSuite * inSuite, void * inContext)
     ExchangeContext * ec1 = ctx.NewExchangeToBob(&sendDelegate);
 
     // Expire the session this exchange is supposedly on.
-    ctx.GetSecureSessionManager().ExpirePairing(ec1->GetSessionHandle());
+    ec1->GetSessionHandle()->AsSecureSession()->MarkForEviction();
 
     MockAppDelegate receiveDelegate;
     CHIP_ERROR err =
@@ -156,7 +156,7 @@ void CheckSessionExpirationTimeout(nlTestSuite * inSuite, void * inContext)
     NL_TEST_ASSERT(inSuite, !sendDelegate.IsOnResponseTimeoutCalled);
 
     // Expire the session this exchange is supposedly on.  This should close the exchange.
-    ctx.GetSecureSessionManager().ExpirePairing(ec1->GetSessionHandle());
+    ec1->GetSessionHandle()->AsSecureSession()->MarkForEviction();
     NL_TEST_ASSERT(inSuite, sendDelegate.IsOnResponseTimeoutCalled);
 
     // recreate closed session.

src/protocols/secure_channel/CASEServer.cpp

@@ -103,6 +103,17 @@ void CASEServer::PrepareForSessionEstablishment(const ScopedNodeId & previouslyE
 
     GetSession().Clear();
 
+    //
+    // This releases our reference to a previously pinned session. If that was a successfully established session and is now
+    // active, this will have no effect (the session will remain in the session table).
+    //
+    // If we previously held a session still in the pairing state, it means PairingSession was owning that session. Since it
+    // gave up its reference by the time we got here, releasing the pinned session here will actually result in it being
+    // de-allocated since no one else is holding onto this session. This will mean that when we get to allocating a session below,
+    // we'll at least have one free session available in the session table, and won't need to evict an arbitrary session.
+    //
+    mPinnedSecureSession.ClearValue();
+
     //
     // Indicate to the underlying CASE session to prepare for session establishment requests coming its way. This will
     // involve allocating a SecureSession that will be held until it's needed for the next CASE session handshake.
@@ -146,7 +157,17 @@ void CASEServer::PrepareForSessionEstablishment(const ScopedNodeId & previouslyE
 void CASEServer::OnSessionEstablishmentError(CHIP_ERROR err)
 {
     ChipLogError(Inet, "CASE Session establishment failed: %s", ErrorStr(err));
-    PrepareForSessionEstablishment();
+
+    //
+    // We're not allowed to call methods that will eventually result in calling SessionManager::AllocateSecureSession
+    // from a SessionDelegate::OnSessionReleased callback. Schedule the preparation as an async work item.
+    //
+    mSessionManager->SystemLayer()->ScheduleWork(
+        [](auto * systemLayer, auto * appState) {
+            CASEServer * _this = static_cast<CASEServer *>(appState);
+            _this->PrepareForSessionEstablishment();
+        },
+        this);
 }
 
 void CASEServer::OnSessionEstablished(const SessionHandle & session)

src/protocols/secure_channel/CASESession.cpp

@@ -175,7 +175,7 @@ CHIP_ERROR
 CASESession::PrepareForSessionEstablishment(SessionManager & sessionManager, FabricTable * fabrics,
                                             SessionResumptionStorage * sessionResumptionStorage,
                                             Credentials::CertificateValidityPolicy * policy,
-                                            SessionEstablishmentDelegate * delegate, ScopedNodeId previouslyEstablishedPeer,
+                                            SessionEstablishmentDelegate * delegate, const ScopedNodeId & previouslyEstablishedPeer,
                                             Optional<ReliableMessageProtocolConfig> mrpConfig)
 {
     VerifyOrReturnError(fabrics != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

src/protocols/secure_channel/CASESession.h

@@ -86,7 +86,7 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
     CHIP_ERROR PrepareForSessionEstablishment(
         SessionManager & sessionManager, FabricTable * fabrics, SessionResumptionStorage * sessionResumptionStorage,
         Credentials::CertificateValidityPolicy * policy, SessionEstablishmentDelegate * delegate,
-        ScopedNodeId previouslyEstablishedPeer,
+        const ScopedNodeId & previouslyEstablishedPeer,
         Optional<ReliableMessageProtocolConfig> mrpConfig = Optional<ReliableMessageProtocolConfig>::Missing());
 
     /**

src/protocols/secure_channel/PairingSession.cpp

@@ -34,14 +34,10 @@ CHIP_ERROR PairingSession::AllocateSecureSession(SessionManager & sessionManager
 
 CHIP_ERROR PairingSession::ActivateSecureSession(const Transport::PeerAddress & peerAddress)
 {
-    // TODO(17568): Replace with proper expiry logic. This current method makes sure there
-    // are not multiple sessions established, until eventual exhaustion of the resources
-    // for CASE sessions. Current method is quick fix for #17698, cannot remain.
-    mSessionManager->ExpireAllPairingsForPeerExceptPending(GetPeer());
-
     // Prepare SecureSession fields, including key derivation, first, before activation
     Transport::SecureSession * secureSession = mSecureSessionHolder->AsSecureSession();
     ReturnErrorOnFailure(DeriveSecureSession(secureSession->GetCryptoContext()));
+
     uint16_t peerSessionId = GetPeerSessionId();
     secureSession->SetPeerAddress(peerAddress);
     secureSession->GetSessionMessageCounter().GetPeerMessageCounter().SetCounter(Transport::PeerMessageCounter::kInitialSyncValue);
@@ -50,7 +46,7 @@ CHIP_ERROR PairingSession::ActivateSecureSession(const Transport::PeerAddress &
     // a partially valid session.
     secureSession->Activate(GetLocalScopedNodeId(), GetPeer(), GetPeerCATs(), peerSessionId, mRemoteMRPConfig);
 
-    ChipLogDetail(Inet, "New secure session created for device " ChipLogFormatScopedNodeId ", LSID:%d PSID:%d!",
+    ChipLogDetail(Inet, "New secure session activated for device " ChipLogFormatScopedNodeId ", LSID:%d PSID:%d!",
                   ChipLogValueScopedNodeId(GetPeer()), secureSession->GetLocalSessionId(), peerSessionId);
 
     return CHIP_NO_ERROR;

src/transport/BUILD.gn

@@ -34,6 +34,7 @@ static_library("transport") {
     "SecureMessageCodec.h",
     "SecureSession.cpp",
     "SecureSession.h",
+    "SecureSessionTable.cpp",
     "SecureSessionTable.h",
     "Session.cpp",
     "Session.h",