pkcs11 store: Improve Store performance through batch sector commits to Store_Close - #873
pkcs11 store: Improve Store performance through batch sector commits to Store_Close#873danielinux wants to merge 2 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.
cd432f3 to
3e0645d
Compare
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
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.
| return -1; | ||
|
|
||
| obj_size = handle->hdr->size; | ||
| obj_size = handle->size; |
There was a problem hiding this comment.
Store_Read pairs a stale size snapshot with live cached data · Logic errors
obj_size is the handle->size snapshot taken at Store_Open, while the payload bytes now come through sector_ptr() (live, including uncommitted cache). When another window truncates the same object, an already-open read handle returns erased 0xFF bytes instead of EOF.
Related known finding #4648 (similar but distinct): Both involve PKCS#11 store read visibility after size changes, but #4648 faults in Store_Write commit/size-update ordering during power loss; this faults in Store_Read combining an Open-time size snapshot with live cached payload after concurrent truncation. The root causes and required patches differ.
Fix: Read the size live via sector0_ptr() + hdr_off in Store_Read/Store_Write instead of the handle->size snapshot.
|
|
||
| /* The write is still pending in the write window. A concurrent | ||
| * reader on the same object must observe it, not erased flash. */ | ||
| ret = wolfPKCS11_Store_Open(type, id_tok, 1, 1, &store_r); |
There was a problem hiding this comment.
Concurrent-reader test never exercises the new cached-read path · Missing edge-case coverage on a function the PR also changed
The reader's Store_Open calls check_vault(), which flushes the whole cache, so sector_ptr() in Store_Read always falls through to flash here. The new cached-read branch of wolfPKCS11_Store_Read stays unexercised and the test passes identically against the pre-PR memcpy.
Fix: Dirty the cache after the reader is open — write on the writer handle between the reader's Open and Read.
|
|
||
| /* The write is still pending in the write window. A concurrent | ||
| * reader on the same object must observe it, not erased flash. */ | ||
| ret = wolfPKCS11_Store_Open(type, id_tok, 1, 1, &store_r); |
There was a problem hiding this comment.
test_concurrent_reader_sees_pending_writes cannot exercise the cache-aware read path it claims to cover · Weak or missing assertions
wolfPKCS11_Store_Open unconditionally calls check_vault() (src/pkcs11_store.c:634), which begins with cache_flush_all() (line 384), so the sector cache is empty when the reader calls wolfPKCS11_Store_Read. sector_ptr() therefore always returns vault_base + offset, and the test passes unchanged with the pre-PR direct memcpy read, leaving the new cache-hit read loop with no coverage.
Fix: Dirty an overlapping sector after the reader is opened (write through a second already-open handle) so sector_ptr() returns a cached sector during wolfPKCS11_Store_Read.
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