pkcs11 store: Improve Store performance through batch sector commits to Store_Close - #873
Open
danielinux wants to merge 6 commits into
Open
pkcs11 store: Improve Store performance through batch sector commits to Store_Close#873danielinux wants to merge 6 commits into
danielinux wants to merge 6 commits into
Conversation
Every wolfPKCS11 field write flushed the payload sector and the header sector to flash (2 erases + 2 programs of a full sector each), and the token store re-serializes all objects per C_CreateObject/C_DestroyObject, so those calls cost hundreds of sector erases and tens of seconds on flash with slow erase times. Cache modified sectors in RAM and commit them together when the store window closes: - sector cache sized to the worst-case span of one object plus the header sector (WOLFBOOT_PKCS11_STORE_CACHE_SECTORS), LRU eviction when exceeded - header sector commits last, so a committed header is the atomic commit point of the batch: power failure during a flush leaves the flash in either the pre-batch or the post-batch state - per-commit backup sector write preserved, keeping recovery of the sector in flight at failure time - delete_object commits on return (durability contract, unit-tested) - nodes table, bitmap, payload ids and the live object size (handle->size) are read from the cache when the sector is dirty Measured on an STM32H5 with 8KB sectors, wolfPKCS11 in the secure world: C_CreateObject 1.5s -> 0.15s, C_DestroyObject 1.3s -> 0.12s, 456 -> 40 sector erases per create, and the count no longer scales with the number of objects in the token. PKCS11_STORE_STATS (off by default) adds flash-activity counters and a test-app bench to quantify store traffic: make PKCS11_STORE_STATS=1.
danielinux
force-pushed
the
pkcs11-store-batch-commit
branch
from
August 25, 2026 14:14
cd432f3 to
3e0645d
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves wolfBoot’s wolfPKCS11 store backend performance by caching sector updates in RAM and committing them in a batch when the store window closes, reducing repeated flash erase/program cycles. It also adds optional instrumentation and a benchmark path to quantify flash traffic reductions.
Changes:
- Implement a multi-sector RAM cache for the PKCS11 store and flush cached sectors on
wolfPKCS11_Store_Close(), committing the header sector last to act as the batch “commit point”. - Track object size live in the store handle (and validate committed size in unit tests).
- Add optional
PKCS11_STORE_STATScounters plus NSC calls and a test-app benchmark harness.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/unit-tests/unit-pkcs11_store.c | Updates assertions to validate live handle size vs committed node size after close. |
| test-app/test_pkcs11.c | Adds optional store-traffic benchmark (guarded by PKCS11_STORE_STATS). |
| test-app/Makefile | Adds PKCS11_STORE_STATS compile flag plumbing for the test app (TZ build). |
| src/pkcs11_store.c | Introduces sector cache + batched commits and live handle size tracking; adds optional flash-activity stats. |
| src/pkcs11_callable.c | Adds NSC wrappers to expose store stats/reset without modifying wolfPKCS11 submodule code. |
| options.mk | Adds global PKCS11_STORE_STATS compile flag plumbing. |
| include/wolfboot/wcs_pkcs11.h | Exposes NSC prototypes for store stats/reset when PKCS11_STORE_STATS is enabled. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
check_vault() dropped the shared sector cache on every vault validation, silently losing the pending writes of any still-open window when another handle was opened or an object removed (MAX_OPEN_STORES allows 16). Flush instead - the atomic header-last commit - so an in-flight batch only gets an earlier commit point; its data is never discarded. wolfPKCS11_Store_Read() now reads through sector_ptr() like every other read in the file, so a sector still in the cache can never be read stale against a live size. Add unit tests covering the interleaved-window data loss and a concurrent reader observing a pending write; both fail without the check_vault fix.
wolfSSL-Fenrir-bot
requested changes
Aug 26, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #873
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Store_Read and Store_Write used handle->size, a snapshot taken at Store_Open. The payload path reads through the sector cache, so once another window's batch (e.g. a write-open truncation) sat pending in the cache, the window saw live erased data under a stale size and returned 0xFF bytes past the true end instead of EOF. Pre-PR the size was read live from the flash header on every call, so the PR regressed that case. Read the size from the same (possibly cached) header sector the payload comes from, via store_live_size(), so size and data share one source of truth. Drop the now-dead handle->size snapshot; update_store_size() only writes the cached header node. Addresses PR wolfSSL#873 review comment (wolfSSL-Fenrir-bot, src/pkcs11_store.c:711).
test_concurrent_reader_sees_pending_writes only ever read from flash: the reader's Store_Open calls check_vault(), which flushes the sector cache, so the sector_ptr() read path in Store_Read was never exercised and the test passed identically against the pre-PR memcpy. Write more on the still-open writer after the reader is open. That batch lands only in the sector cache, so the reader can only see it through the cached read path and the live header size; a flash-only or snapshot-size read returns EOF here. Verified: the new assertion fails against the pre-fix store (ret == 0) and passes with the live-size fix. Addresses PR wolfSSL#873 review comments (wolfSSL-Fenrir-bot, tools/unit-tests/unit-pkcs11_store.c:587, both near-duplicate findings).
wolfSSL-Fenrir-bot
requested changes
Aug 31, 2026
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #873
Scan targets checked: wolfboot-bugs, wolfboot-src
Findings: 3
3 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
…bility) Three fixes from the 2026-08-31 Fenrir PR review round: 1. cache_get_sector() LRU eviction could pick the header sector (offset 0) as the victim, committing it to flash while the batch's payload sectors were still only in RAM - a mixed pre/post-batch state that breaks the header-last atomic commit point cache_flush_all() relies on. The header is now exempt from victim selection; if it is the only cached sector, flush the whole batch instead (header-last is then trivial). 2. cache_flush_all() and LRU eviction released a slot by clearing .sector without wiping the buffer, so private-key bytes staged by Store_Write lingered in secure-world SRAM until the slot was next reused. The single staging buffer this cache replaced self-cleaned (the header sector overwrote it on every size update); per-sector slots do not. A new cache_release() wc_ForceZero()s the buffer before freeing the slot. 3. Store_Open in write mode set the size to 8 (truncation) in the cache only; with batched commits the payload sectors are flushed before the header, so a power loss during the erase/rewrite left the old size over a partly erased payload. The truncated header is now committed to flash before erase_object_payload(), so the empty state is the crash fallback. Addresses PR wolfSSL#873 review comments (wolfSSL-Fenrir-bot, src/pkcs11_store.c:301, :333, :670, 2026-08-31). Verification: tools/unit-tests unit-pkcs11_store 9/9 pass (incl. test_concurrent_reader_sees_pending_writes, test_shorter_overwrite_erases_residual_key_material, test_interleaved_write_windows_both_persist).
Adds power-fail injection to the flash mock and a test that cuts power at every flash operation of a rewrite window, asserting the object always reads back as the whole old payload, the whole new payload, or empty. Fails at op 3 without this fix, passes with it. unit-pkcs11_store 10/10; full unit-tests suite green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Every wolfPKCS11 field write flushed the payload sector and the header sector to flash (2 erases + 2 programs of a full sector each), and the token store re-serializes all objects per C_CreateObject/C_DestroyObject, so those calls cost hundreds of sector erases and tens of seconds on flash with slow erase times.
Cache modified sectors in RAM and commit them together when the store window closes:
Testing
Measured on an STM32H5 with 8KB sectors, wolfPKCS11 in the secure world: C_CreateObject 1.5s -> 0.15s, C_DestroyObject 1.3s -> 0.12s, 456 -> 40 sector erases per create, and the count no longer scales with the number of objects in the token.
PKCS11_STORE_STATS (off by default) adds flash-activity counters and a test-app bench to quantify store traffic: make PKCS11_STORE_STATS=1.
Should fix ZD-21908