[SL-UP] Fix NVM3 Key leak on reboot (#464) (#39671)
diff --git a/examples/platform/silabs/BaseApplication.cpp b/examples/platform/silabs/BaseApplication.cpp
index a662f35..a6f24c5 100644
--- a/examples/platform/silabs/BaseApplication.cpp
+++ b/examples/platform/silabs/BaseApplication.cpp
@@ -882,10 +882,16 @@
void BaseApplication::DoProvisioningReset()
{
PlatformMgr().ScheduleWork([](intptr_t) {
+ // Force the KeyMap update to make sure nvm3 is updated before anything happens.
+ // If the device reboots before the timer update happens, "shadow" keys are left in nvm3 causing a reduction of the
+ // overall available nvm3 - similar to a memory leak.
+ // FactoryResetThreadStack forces a reboot which was causing a memory loss.
+ chip::DeviceLayer::PersistedStorage::KeyValueStoreMgrImpl().ForceKeyMapSave();
+
#if CHIP_DEVICE_CONFIG_ENABLE_THREAD
ConfigurationManagerImpl::GetDefaultInstance().ClearThreadStack();
ThreadStackMgrImpl().FactoryResetThreadStack();
- ThreadStackMgr().InitThreadStack();
+ // Triggers a reboot - nothing gets executed after this
#endif // CHIP_DEVICE_CONFIG_ENABLE_THREAD
#if CHIP_DEVICE_CONFIG_ENABLE_WIFI_STATION
diff --git a/src/platform/silabs/KeyValueStoreManagerImpl.cpp b/src/platform/silabs/KeyValueStoreManagerImpl.cpp
index 34d8838..9ea9e36 100644
--- a/src/platform/silabs/KeyValueStoreManagerImpl.cpp
+++ b/src/platform/silabs/KeyValueStoreManagerImpl.cpp
@@ -22,6 +22,7 @@
*/
#include "MigrationManager.h"
+#include <cmsis_os2.h>
#include <crypto/CHIPCryptoPAL.h>
#include <platform/CHIPDeviceLayer.h>
#include <platform/KeyValueStoreManager.h>
@@ -40,29 +41,38 @@
namespace DeviceLayer {
namespace PersistedStorage {
-KeyValueStoreManagerImpl KeyValueStoreManagerImpl::sInstance;
+namespace {
+
uint16_t mKvsKeyMap[KeyValueStoreManagerImpl::kMaxEntries] = { 0 };
+} // namespace
+
+KeyValueStoreManagerImpl KeyValueStoreManagerImpl::sInstance;
+
CHIP_ERROR KeyValueStoreManagerImpl::Init(void)
{
- CHIP_ERROR err;
- err = SilabsConfig::Init();
- SuccessOrExit(err);
+ ReturnErrorOnFailure(SilabsConfig::Init());
Silabs::MigrationManager::GetMigrationInstance().applyMigrations();
memset(mKvsKeyMap, 0, sizeof(mKvsKeyMap));
- size_t outLen;
- err = SilabsConfig::ReadConfigValueBin(SilabsConfig::kConfigKey_KvsStringKeyMap, reinterpret_cast<uint8_t *>(mKvsKeyMap),
- sizeof(mKvsKeyMap), outLen);
-
- if (err == CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND) // Initial boot
+ size_t outLen = 0;
+ CHIP_ERROR error = SilabsConfig::ReadConfigValueBin(SilabsConfig::kConfigKey_KvsStringKeyMap,
+ reinterpret_cast<uint8_t *>(mKvsKeyMap), sizeof(mKvsKeyMap), outLen);
+ if (error == CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND) // Initial boot
{
- err = CHIP_NO_ERROR;
+ error = CHIP_NO_ERROR;
+ }
+ ReturnErrorOnFailure(error);
+
+ constexpr osThreadAttr_t attr = { .name = "KvsKeyMapCleanupTask", .stack_size = 512 /* bytes */, .priority = osPriorityLow7 };
+ if (osThreadNew(KvsKeyMapCleanup, nullptr, &attr) == nullptr)
+ {
+ // We don't need to crash and force a reboot. we will retry at the next reboot
+ ChipLogError(DeviceLayer, "Failed to create KvsCleanupTask");
}
-exit:
- return err;
+ return CHIP_NO_ERROR;
}
bool KeyValueStoreManagerImpl::IsValidKvsNvm3Key(uint32_t nvm3Key) const
@@ -109,7 +119,15 @@
// Collision prevention
// Read the data from NVM3 it should be prefixed by the kvsString
// else we will look for another matching hash in the map
- SilabsConfig::ReadConfigValueBin(tempNvm3key, reinterpret_cast<uint8_t *>(strPrefix), length, readCount, 0);
+
+ CHIP_ERROR err =
+ SilabsConfig::ReadConfigValueBin(tempNvm3key, reinterpret_cast<uint8_t *>(strPrefix), length, readCount, 0);
+ if (err == CHIP_DEVICE_ERROR_CONFIG_NOT_FOUND)
+ {
+ // No steps to be taken, clean up will be done at init
+ ChipLogError(DeviceLayer, "Key Hash with no associated NVM3 entry - potential Shadow key detected");
+ }
+
if (strcmp(key, strPrefix) == 0)
{
// String matches we have confirmed the hash pointed us the right key data
@@ -299,6 +317,34 @@
}
}
+void KeyValueStoreManagerImpl::KvsKeyMapCleanup(void * argument)
+{
+ bool requireKvsKeyMapSave = false;
+
+ for (uint16_t key = 0; key < KeyValueStoreManagerImpl::kMaxEntries; key++)
+ {
+ // Only check the keys that should have a NVM entry and
+ // we don't need to take a mutex - protection is done by the underlying nvm3 APIs called in ConfigValueExists
+ if (mKvsKeyMap[key] != 0 && !SilabsConfig::ConfigValueExists(CONVERT_KEYMAP_INDEX_TO_NVM3KEY(key)))
+ {
+ // We have found a shadow key (key with no NVM entry)
+ // Delete unused key to free up space
+ mKvsKeyMap[key] = 0;
+ requireKvsKeyMapSave = true;
+ }
+ }
+
+ if (requireKvsKeyMapSave)
+ {
+ PlatformMgr().LockChipStack();
+ KeyValueStoreMgrImpl().ForceKeyMapSave();
+ PlatformMgr().UnlockChipStack();
+ }
+
+ // Single shot task at boot - delete the task when the clean up is complete
+ osThreadExit();
+}
+
} // namespace PersistedStorage
namespace Silabs {
diff --git a/src/platform/silabs/KeyValueStoreManagerImpl.h b/src/platform/silabs/KeyValueStoreManagerImpl.h
index c17fad8..2cf20a7 100644
--- a/src/platform/silabs/KeyValueStoreManagerImpl.h
+++ b/src/platform/silabs/KeyValueStoreManagerImpl.h
@@ -46,8 +46,19 @@
static void KvsMapMigration();
private:
+ static KeyValueStoreManagerImpl sInstance;
+
static void OnScheduledKeyMapSave(System::Layer * systemLayer, void * appState);
+ /**
+ * @brief Cleans up unused keys in the key-value store.
+ *
+ * This function iterates over the key map and removes shadow keys (keys that
+ * no longer have corresponding entries in NVM). It ensures that the key map
+ * remains consistent and frees up space for new entries.
+ */
+ static void KvsKeyMapCleanup(void * argument);
+
void ScheduleKeyMapSave(void);
bool IsValidKvsNvm3Key(const uint32_t nvm3Key) const;
uint16_t hashKvsKeyString(const char * key) const;
@@ -56,8 +67,6 @@
// ===== Members for internal use by the following friends.
friend KeyValueStoreManager & KeyValueStoreMgr();
friend KeyValueStoreManagerImpl & KeyValueStoreMgrImpl();
-
- static KeyValueStoreManagerImpl sInstance;
};
/**
@@ -75,7 +84,7 @@
* Returns the platform-specific implementation of the KeyValueStoreManager singleton object.
*
* Chip applications can use this to gain access to features of the KeyValueStoreManager
- * that are specific to the ESP32 platform.
+ * that are specific to the Silabs platform.
*/
inline KeyValueStoreManagerImpl & KeyValueStoreMgrImpl(void)
{
