pkcs11 store: Improve Store performance through batch sector commits to Store_Close - #873
Merged
Merged
Conversation
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.
wolfSSL-Fenrir-bot
previously 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.
danielinux
added a commit
to danielinux/wolfBoot
that referenced
this pull request
Aug 31, 2026
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).
danielinux
added a commit
to danielinux/wolfBoot
that referenced
this pull request
Aug 31, 2026
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
previously 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.
danielinux
added a commit
to danielinux/wolfBoot
that referenced
this pull request
Aug 31, 2026
…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).
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.
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.
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).
…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.
The store commits sectors with hal_flash_erase()/hal_flash_write() and then reads them back through the memory map - sector_ptr(), cache_get_sector()'s refill, and the raw magic reads in check_vault(). On a part that caches flash reads (STM32 ICACHE) those reads can return pre-erase bytes. check_vault() is the worst case: a stale magic there does not merely read wrong, it triggers restore_backup() or a full vault re-initialisation, losing the token.
danielinux
force-pushed
the
pkcs11-store-batch-commit
branch
from
September 10, 2026 08:46
156ce6a to
42e6dba
Compare
The weak no-op sat inside NVM_FLASH_WRITEONCE, so pkcs11_store.c failed to link on the nrf5340/nrf54l TrustZone configs. STM32F4 and STM32G4 enable the flash instruction/data caches and never reset them, so give them a real one.
danielinux
requested review from
wolfSSL-Fenrir-bot
and removed request for
wolfSSL-Fenrir-bot
September 10, 2026 16:59
wolfSSL-Fenrir-bot
previously requested changes
Sep 10, 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-src, wolfboot-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
vault_obj_read() now separates "object absent" from a failed read, so the power-fail test rejects a corrupted vault instead of accepting any negative. hal_cache_invalidate() shrank 120 -> 44 bytes; the 48 it still costs every STM32F407 build is folded into the test-size-all limits.
wolfSSL-Fenrir-bot
previously requested changes
Sep 10, 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-src, wolfboot-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
…r fail Under MOCK_STALE_CACHE the shadow is the flash array and vault_base only the CPU's view, so restoring vault_base alone let the power cycle copy the last iteration's result back: no crash case started from the old generation.
wolfSSL-Fenrir-bot
previously requested changes
Sep 10, 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-src, wolfboot-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
The crash == ops iteration injects nothing, so it must read back new_p; the empty/absent branch was letting a silently lost complete rewrite pass. Also assert the uninjected baseline write lands before measuring against it.
wolfSSL-Fenrir-bot
previously requested changes
Sep 10, 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-src, wolfboot-bugs
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
…eader The mock abandoned a faulting erase/program whole, so half-written sectors went untested; it can now tear one part-way and the sweep runs every crash point at 1/4, 1/2 and 3/4. An empty read must also leave the node in place with size 8, so a lost or torn header no longer passes as an empty rewrite.
wolfSSL-Fenrir-bot
dismissed
their stale review
September 10, 2026 19:44
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
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-src, wolfboot-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
wolfSSL-Fenrir-bot
dismissed stale reviews from themself
September 10, 2026 19:44
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
dgarske
self-requested a review
September 10, 2026 21:34
dgarske
approved these changes
Sep 10, 2026
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