Merge branch 'bugfix/nvs_lock_initi_and_multipage_blob_v5.1' into 'release/v5.1'

Bugfix/nvs Improved handling of BLOB during unreliable power environment and concurrent data access scenarios (v5.1)

See merge request espressif/esp-idf!29321

components/nvs_flash/CMakeLists.txt

@@ -1,5 +1,11 @@
 idf_build_get_property(target IDF_TARGET)
 
+if(${target} STREQUAL "linux")
+# omit newlib for linux
+else()
+    list(APPEND priv_requires "newlib")
+endif()
+
 set(srcs "src/nvs_api.cpp"
          "src/nvs_cxx_api.cpp"
          "src/nvs_item_hash_list.cpp"
@@ -11,13 +17,13 @@ set(srcs "src/nvs_api.cpp"
          "src/nvs_partition.cpp"
          "src/nvs_partition_lookup.cpp"
          "src/nvs_partition_manager.cpp"
-         "src/nvs_types.cpp")
+         "src/nvs_types.cpp"
+         "src/nvs_platform.cpp")
 
 idf_component_register(SRCS "${srcs}"
-                    REQUIRES "esp_partition"
+                    REQUIRES "esp_partition" "spi_flash"
-                    PRIV_REQUIRES spi_flash
+                    PRIV_REQUIRES "${priv_requires}"
                     INCLUDE_DIRS "include"
-                    "../spi_flash/include"
                     PRIV_INCLUDE_DIRS "private_include")
 
 # If we use the linux target, we need to redirect the crc functions to the linux

components/nvs_flash/src/nvs_api.cpp

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -45,10 +45,6 @@ uint32_t NVSHandleEntry::s_nvs_next_handle;
 
 extern "C" void nvs_dump(const char *partName);
 
-#ifndef LINUX_TARGET
-SemaphoreHandle_t nvs::Lock::mSemaphore = nullptr;
-#endif // ! LINUX_TARGET
-
 using namespace std;
 using namespace nvs;
 
@@ -272,6 +268,10 @@ static esp_err_t nvs_find_ns_handle(nvs_handle_t c_handle, NVSHandleSimple** han
 
 extern "C" esp_err_t nvs_open_from_partition(const char *part_name, const char* namespace_name, nvs_open_mode_t open_mode, nvs_handle_t *out_handle)
 {
+    esp_err_t lock_result = Lock::init();
+    if (lock_result != ESP_OK) {
+        return lock_result;
+    }
     Lock lock;
     ESP_LOGD(TAG, "%s %s %d", __func__, namespace_name, open_mode);
 

components/nvs_flash/src/nvs_page.cpp

@@ -878,7 +878,10 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si
         end = ENTRY_COUNT;
     }
 
-    if (nsIndex != NS_ANY && datatype != ItemType::ANY && key != NULL) {
+    // For BLOB_DATA, we may need to search for all chunk indexes, so the hash list won't help
+    // mHashIndex caluclates hash from nsIndex, key, chunkIdx
+    // We may not use mHashList if datatype is BLOB_DATA and chunkIdx is CHUNK_ANY as CHUNK_ANY is used by BLOB_INDEX
+    if (nsIndex != NS_ANY && key != NULL && (datatype != ItemType::BLOB_DATA || chunkIdx != CHUNK_ANY)) {
         size_t cachedIndex = mHashList.find(start, Item(nsIndex, datatype, 0, key, chunkIdx));
         if (cachedIndex < ENTRY_COUNT) {
             start = cachedIndex;
@@ -933,6 +936,31 @@ esp_err_t Page::findItem(uint8_t nsIndex, ItemType datatype, const char* key, si
                 && item.chunkIndex != chunkIdx) {
             continue;
         }
+
+        // We may search for any chunk of BLOB_DATA but find BLOB_INDEX or BLOB instead as it
+        // uses default value of chunkIdx == CHUNK_ANY, then continue searching
+        if (chunkIdx == CHUNK_ANY
+                && datatype == ItemType::BLOB_DATA
+                && item.datatype != ItemType::BLOB_DATA) {
+            continue;
+        }
+
+        // We may search for BLOB but find BLOB_INDEX instead
+        // In this case it is expected to return ESP_ERR_NVS_TYPE_MISMATCH
+        if (chunkIdx == CHUNK_ANY
+                && datatype == ItemType::BLOB
+                && item.datatype == ItemType::BLOB_IDX) {
+            return ESP_ERR_NVS_TYPE_MISMATCH;
+        }
+
+        // We may search for BLOB but find BLOB_DATA instead
+        // Then continue
+        if (chunkIdx == CHUNK_ANY
+                && datatype == ItemType::BLOB
+                && item.datatype == ItemType::BLOB_DATA) {
+            continue;
+        }
+
         /* Blob-index will match the <ns,key> with blob data.
          * Skip data chunks when searching for blob index*/
         if (datatype == ItemType::BLOB_IDX

components/nvs_flash/src/nvs_page.hpp

@@ -98,6 +98,8 @@ public:
 
     esp_err_t findItem(uint8_t nsIndex, ItemType datatype, const char* key, size_t &itemIndex, Item& item, uint8_t chunkIdx = CHUNK_ANY, VerOffset chunkStart = VerOffset::VER_ANY);
 
+    esp_err_t eraseEntryAndSpan(size_t index);
+
     template<typename T>
     esp_err_t writeItem(uint8_t nsIndex, const char* key, const T& value)
     {
@@ -188,8 +190,6 @@ protected:
 
     esp_err_t writeEntryData(const uint8_t* data, size_t size);
 
-    esp_err_t eraseEntryAndSpan(size_t index);
-
     esp_err_t updateFirstUsedEntry(size_t index, size_t span);
 
     static constexpr size_t getAlignmentForType(ItemType type)

components/nvs_flash/src/nvs_platform.cpp

@@ -0,0 +1,50 @@
+/*
+ * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+#include "nvs_platform.hpp"
+
+using namespace nvs;
+
+#ifdef LINUX_TARGET
+Lock::Lock() {}
+Lock::~Lock() {}
+esp_err_t nvs::Lock::init() {return ESP_OK;}
+void Lock::uninit() {}
+#else
+
+#include "sys/lock.h"
+
+Lock::Lock()
+{
+    // Newlib implementation ensures that even if mSemaphore was 0, it gets initialized.
+    // Locks mSemaphore
+    _lock_acquire(&mSemaphore);
+}
+
+Lock::~Lock()
+{
+    // Unlocks mSemaphore
+    _lock_release(&mSemaphore);
+}
+
+esp_err_t Lock::init()
+{
+    // Let postpone initialization to the Lock::Lock.
+    // It is designed to lazy initialize the semaphore in a properly guarded critical section
+    return ESP_OK;
+}
+
+void Lock::uninit()
+{
+    // Uninitializes mSemaphore. Please be aware that uninitialization of semaphore shared and held by another thread
+    // can cause undefined behavior
+    if (mSemaphore) {
+        _lock_close(&mSemaphore);
+    }
+}
+
+_lock_t Lock::mSemaphore = 0;
+
+#endif

components/nvs_flash/src/nvs_platform.hpp

@@ -1,82 +1,24 @@
-// Copyright 2015-2016 Espressif Systems (Shanghai) PTE LTD
+/*
-//
+ * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
-// Licensed under the Apache License, Version 2.0 (the "License");
+ *
-// you may not use this file except in compliance with the License.
+ * SPDX-License-Identifier: Apache-2.0
-// You may obtain a copy of the License at
+ */
-
-//     http://www.apache.org/licenses/LICENSE-2.0
-//
-// Unless required by applicable law or agreed to in writing, software
-// distributed under the License is distributed on an "AS IS" BASIS,
-// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-// See the License for the specific language governing permissions and
-// limitations under the License.
 #pragma once
 
-#ifdef LINUX_TARGET
+#include "esp_err.h"
-namespace nvs
-{
-class Lock
-{
-public:
-    Lock() { }
-    ~Lock() { }
-    static esp_err_t init()
-    {
-        return ESP_OK;
-    }
-
-    static void uninit() {}
-};
-} // namespace nvs
-
-#else // LINUX_TARGET
-
-#include "freertos/FreeRTOS.h"
-#include "freertos/semphr.h"
 
 namespace nvs
 {
-
+    class Lock
-class Lock
-{
-public:
-    Lock()
     {
-        if (mSemaphore) {
+    public:
-            xSemaphoreTake(mSemaphore, portMAX_DELAY);
+        Lock();
-        }
+        ~Lock();
-    }
+        static esp_err_t init();
-
+        static void uninit();
-    ~Lock()
+#ifndef LINUX_TARGET
-    {
+    private:
-        if (mSemaphore) {
+        static _lock_t mSemaphore;
-            xSemaphoreGive(mSemaphore);
+#endif
-        }
+    };
-    }
-
-    static esp_err_t init()
-    {
-        if (mSemaphore) {
-            return ESP_OK;
-        }
-        mSemaphore = xSemaphoreCreateMutex();
-        if (!mSemaphore) {
-            return ESP_ERR_NO_MEM;
-        }
-        return ESP_OK;
-    }
-
-    static void uninit()
-    {
-        if (mSemaphore) {
-            vSemaphoreDelete(mSemaphore);
-        }
-        mSemaphore = nullptr;
-    }
-
-    static SemaphoreHandle_t mSemaphore;
-};
 } // namespace nvs
-
-#endif // LINUX_TARGET

components/nvs_flash/src/nvs_storage.cpp

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -20,6 +20,9 @@
 #endif
 #endif // !ESP_PLATFORM
 
+#include "esp_log.h"
+#define TAG "nvs_storage"
+
 namespace nvs
 {
 
@@ -53,6 +56,9 @@ esp_err_t Storage::populateBlobIndices(TBlobIndexList& blobIdxList)
             entry->nsIndex = item.nsIndex;
             entry->chunkStart = item.blobIndex.chunkStart;
             entry->chunkCount = item.blobIndex.chunkCount;
+            entry->dataSize = item.blobIndex.dataSize;
+            entry->observedDataSize = 0;
+            entry->observedChunkCount = 0;
 
             blobIdxList.push_back(entry);
             itemIndex += item.span;
@@ -62,6 +68,76 @@ esp_err_t Storage::populateBlobIndices(TBlobIndexList& blobIdxList)
     return ESP_OK;
 }
 
+// Check BLOB_DATA entries belonging to BLOB_INDEX entries for mismatched records.
+// BLOB_INDEX record is compared with information collected from BLOB_DATA records
+// matched using namespace index, key and chunk version. Mismatched summary length
+// or wrong number of chunks are checked. Mismatched BLOB_INDEX data are deleted
+// and removed from the blobIdxList. The BLOB_DATA are left as orphans and removed
+// later by the call to eraseOrphanDataBlobs().
+void Storage::eraseMismatchedBlobIndexes(TBlobIndexList& blobIdxList)
+{
+    for (auto it = mPageManager.begin(); it != mPageManager.end(); ++it) {
+        Page& p = *it;
+        size_t itemIndex = 0;
+        Item item;
+        /* Chunks with same <ns,key> and with chunkIndex in the following ranges
+         * belong to same family.
+         * 1) VER_0_OFFSET <= chunkIndex < VER_1_OFFSET-1 => Version0 chunks
+         * 2) VER_1_OFFSET <= chunkIndex < VER_ANY => Version1 chunks
+         */
+        while (p.findItem(Page::NS_ANY, ItemType::BLOB_DATA, nullptr, itemIndex, item) == ESP_OK) {
+
+            auto iter = std::find_if(blobIdxList.begin(),
+                    blobIdxList.end(),
+                    [=] (const BlobIndexNode& e) -> bool
+                    {return (strncmp(item.key, e.key, sizeof(e.key) - 1) == 0)
+                            && (item.nsIndex == e.nsIndex)
+                            && (item.chunkIndex >=  static_cast<uint8_t> (e.chunkStart))
+                            && (item.chunkIndex < static_cast<uint8_t> ((e.chunkStart == nvs::VerOffset::VER_0_OFFSET) ? nvs::VerOffset::VER_1_OFFSET : nvs::VerOffset::VER_ANY));});
+            if (iter != std::end(blobIdxList)) {
+                // accumulate the size
+                iter->observedDataSize += item.varLength.dataSize;
+                iter->observedChunkCount++;
+            }
+            itemIndex += item.span;
+        }
+    }
+
+    auto iter = blobIdxList.begin();
+    while (iter != blobIdxList.end())
+    {
+        if ( (iter->observedDataSize != iter->dataSize) || (iter->observedChunkCount != iter->chunkCount) )
+        {
+            // Delete blob_index from flash
+            // This is very rare case, so we can loop over all pages
+            for (auto it = mPageManager.begin(); it != mPageManager.end(); ++it) {
+                // skip pages in non eligible states
+                if (it->state() == nvs::Page::PageState::CORRUPT
+                    || it->state() == nvs::Page::PageState::INVALID
+                    || it->state() == nvs::Page::PageState::UNINITIALIZED){
+                    continue;
+                }
+
+                Page& p = *it;
+                if(p.eraseItem(iter->nsIndex, nvs::ItemType::BLOB_IDX, iter->key, 255, iter->chunkStart) == ESP_OK){
+                    break;
+                }
+            }
+
+            // Delete blob index from the blobIdxList
+            auto tmp = iter;
+            ++iter;
+            blobIdxList.erase(tmp);
+            delete (nvs::Storage::BlobIndexNode*)tmp;
+        }
+        else
+        {
+            // Blob index OK
+            ++iter;
+        }
+    }
+}
+
 void Storage::eraseOrphanDataBlobs(TBlobIndexList& blobIdxList)
 {
     for (auto it = mPageManager.begin(); it != mPageManager.end(); ++it) {
@@ -85,6 +161,7 @@ void Storage::eraseOrphanDataBlobs(TBlobIndexList& blobIdxList)
             if (iter == std::end(blobIdxList)) {
                 p.eraseItem(item.nsIndex, item.datatype, item.key, item.chunkIndex);
             }
+
             itemIndex += item.span;
         }
     }
@@ -133,7 +210,6 @@ esp_err_t Storage::init(uint32_t baseSector, uint32_t sectorCount)
     if (mNamespaceUsage.set(255, true) != ESP_OK) {
         return ESP_FAIL;
     }
-    mState = StorageState::ACTIVE;
 
     // Populate list of multi-page index entries.
     TBlobIndexList blobIdxList;
@@ -143,12 +219,17 @@ esp_err_t Storage::init(uint32_t baseSector, uint32_t sectorCount)
         return ESP_ERR_NO_MEM;
     }
 
+    // remove blob indexes with mismatched blob data length or chunk count
+    eraseMismatchedBlobIndexes(blobIdxList);
+
     // Remove the entries for which there is no parent multi-page index.
     eraseOrphanDataBlobs(blobIdxList);
 
     // Purge the blob index list
     blobIdxList.clearAndFreeNodes();
 
+    mState = StorageState::ACTIVE;
+
 #ifdef DEBUG_STORAGE
     debugCheck();
 #endif
@@ -490,6 +571,11 @@ esp_err_t Storage::readMultiPageBlob(uint8_t nsIndex, const char* key, void* dat
             }
             return err;
         }
+        if (item.varLength.dataSize > dataSize - offset) {
+            /* The size of the entry in the index is inconsistent with the sum of the sizes of chunks */
+            err = ESP_ERR_NVS_INVALID_LENGTH;
+            break;
+        }
         err = findPage->readItem(nsIndex, ItemType::BLOB_DATA, key, static_cast<uint8_t*>(data) + offset, item.varLength.dataSize, static_cast<uint8_t> (chunkStart) + chunkNum);
         if (err != ESP_OK) {
             return err;
@@ -498,11 +584,14 @@ esp_err_t Storage::readMultiPageBlob(uint8_t nsIndex, const char* key, void* dat
 
         offset += item.varLength.dataSize;
     }
+
+    if (err == ESP_ERR_NVS_NOT_FOUND || err == ESP_ERR_NVS_INVALID_LENGTH) {
+        // cleanup if a chunk is not found or the size is inconsistent
+        eraseMultiPageBlob(nsIndex, key);
+    }
+
     NVS_ASSERT_OR_RETURN(offset == dataSize, ESP_FAIL);
 
-    if (err == ESP_ERR_NVS_NOT_FOUND) {
-        eraseMultiPageBlob(nsIndex, key); // cleanup if a chunk is not found
-    }
     return err;
 }
 
@@ -583,34 +672,57 @@ esp_err_t Storage::eraseMultiPageBlob(uint8_t nsIndex, const char* key, VerOffse
     if (err != ESP_OK) {
         return err;
     }
-    /* Erase the index first and make children blobs orphan*/
+    // Erase the index first and make children blobs orphan
     err = findPage->eraseItem(nsIndex, ItemType::BLOB_IDX, key, Page::CHUNK_ANY, chunkStart);
     if (err != ESP_OK) {
         return err;
     }
 
-    uint8_t chunkCount = item.blobIndex.chunkCount;
+    // If caller requires delete of VER_ANY
-
+    // We may face dirty NVS partition and version duplicates can be there
-    if (chunkStart == VerOffset::VER_ANY) {
+    // Make second attempt to delete index and ignore eventual not found
-        chunkStart = item.blobIndex.chunkStart;
+    if(chunkStart == VerOffset::VER_ANY)
-    } else {
+    {
-        NVS_ASSERT_OR_RETURN(chunkStart == item.blobIndex.chunkStart, ESP_FAIL);
+        err = findItem(nsIndex, ItemType::BLOB_IDX, key, findPage, item, Page::CHUNK_ANY, chunkStart);
+        if (err == ESP_OK) {
+            err = findPage->eraseItem(nsIndex, ItemType::BLOB_IDX, key, Page::CHUNK_ANY, chunkStart);
+            if (err != ESP_OK) {
+                return err;
+            }
+        } else if (err != ESP_ERR_NVS_NOT_FOUND) {
+            return err;
+        }
     }
 
-    /* Now erase corresponding chunks*/
+    // setup limits for chunkIndex-es to be deleted
-    for (uint8_t chunkNum = 0; chunkNum < chunkCount; chunkNum++) {
+    uint8_t minChunkIndex = (uint8_t) VerOffset::VER_0_OFFSET;
-        err = findItem(nsIndex, ItemType::BLOB_DATA, key, findPage, item, static_cast<uint8_t> (chunkStart) + chunkNum);
+    uint8_t maxChunkIndex = (uint8_t) VerOffset::VER_ANY;
 
-        if (err != ESP_OK && err != ESP_ERR_NVS_NOT_FOUND) {
+    if(chunkStart == VerOffset::VER_0_OFFSET) {
-            return err;
+        maxChunkIndex = (uint8_t) VerOffset::VER_1_OFFSET;
-        } else if (err == ESP_ERR_NVS_NOT_FOUND) {
+    } else if (chunkStart == VerOffset::VER_1_OFFSET) {
-            continue; // Keep erasing other chunks
+        minChunkIndex = (uint8_t) VerOffset::VER_1_OFFSET;
-        }
+    }
-        err = findPage->eraseItem(nsIndex, ItemType::BLOB_DATA, key, static_cast<uint8_t> (chunkStart) + chunkNum);
-        if (err != ESP_OK) {
-            return err;
-        }
 
+    for (auto it = std::begin(mPageManager); it != std::end(mPageManager); ++it) {
+        size_t itemIndex = 0;
+        do {
+            err = it->findItem(nsIndex, ItemType::BLOB_DATA, key, itemIndex, item);
+            if (err == ESP_ERR_NVS_NOT_FOUND) {
+                break;
+            } else if (err == ESP_OK) {
+                // check if item.chunkIndex is within the version range indicated by chunkStart, if so, delete it
+                if((item.chunkIndex >= minChunkIndex) && (item.chunkIndex < maxChunkIndex)) {
+                    err = it->eraseEntryAndSpan(itemIndex);
+                }
+
+                // continue findItem until end of page
+                itemIndex += item.span;
+            }
+            if(err != ESP_OK) {
+                return err;
+            }
+        } while (err == ESP_OK && itemIndex < Page::ENTRY_COUNT);
     }
 
     return ESP_OK;

components/nvs_flash/src/nvs_storage.hpp

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -48,6 +48,9 @@ class Storage : public intrusive_list_node<Storage>, public ExceptionlessAllocat
             uint8_t nsIndex;
             uint8_t chunkCount;
             VerOffset chunkStart;
+            size_t dataSize;
+            size_t observedDataSize;
+            size_t observedChunkCount;
     };
 
     typedef intrusive_list<BlobIndexNode> TBlobIndexList;
@@ -140,6 +143,8 @@ protected:
 
     esp_err_t populateBlobIndices(TBlobIndexList&);
 
+    void eraseMismatchedBlobIndexes(TBlobIndexList&);
+
     void eraseOrphanDataBlobs(TBlobIndexList&);
 
     void fillEntryInfo(Item &item, nvs_entry_info_t &info);

components/nvs_flash/test_nvs_host/Makefile

@@ -15,6 +15,7 @@ SOURCE_FILES = \
 		nvs_partition.cpp \
 		nvs_encrypted_partition.cpp \
 		nvs_cxx_api.cpp \
+		nvs_platform.cpp \
 	) \
 	spi_flash_emulation.cpp \
 	test_compressed_enum_table.cpp \

components/spi_flash/CMakeLists.txt

@@ -1,5 +1,7 @@
 idf_build_get_property(target IDF_TARGET)
 if(${target} STREQUAL "linux")
+    idf_component_register(INCLUDE_DIRS include
+                        PRIV_INCLUDE_DIRS include/spi_flash)
     return()
 endif()
 

tools/ci/check_copyright_ignore.txt

@@ -718,7 +718,6 @@ components/nvs_flash/src/nvs_pagemanager.hpp
 components/nvs_flash/src/nvs_partition.cpp
 components/nvs_flash/src/nvs_partition_lookup.cpp
 components/nvs_flash/src/nvs_partition_lookup.hpp
-components/nvs_flash/src/nvs_platform.hpp
 components/nvs_flash/src/nvs_test_api.h
 components/nvs_flash/src/nvs_types.cpp
 components/nvs_flash/src/partition.hpp