fix(prim): refuse a region that cannot hold a segment, and diagnose reentrancy - #19
Merged
Merged
Conversation
…eentrancy Four findings from the first outside firmware to put 2.0.0 on a chip (docs/plans/embedded-adoption.md). All three of its claims verified against the code before anything changed; all three were accurate, and §1b was worse than reported. DOCS FIRST (§1b). `ra_small_profile` appeared ZERO times in either README while `68 KiB` appeared five times as the embedded floor -- a figure only true under that cfg. The README did not merely omit the flag, it handed a reader a number that is wrong without it. Both READMEs now carry "What a firmware has to set", and the footprint row carries the cfg inline. §1. `init_region` validated `len < FIXED_PAGE` and nothing else, so a region that could never yield a segment returned `Ok(())`, linked clean, and failed every allocation on the board. It now returns `FERR_GEOMETRY`. The proposed check -- `len < FIXED_PAGE + SEGMENT_SIZE` -- was NOT sufficient. `init_region` has the base address, and a segment can only start on a segment boundary, so a region passing that length test with a misaligned base still yields nothing. The shipped check asks `usable_bytes(base, len) == 0`, which is exact where a length test is optimistic. `PrimError` is a `u32`, so distinct codes were free: FERR_TOO_SMALL, FERR_GEOMETRY, FERR_REGISTERED. `MIN_REGION` is public so the `const _: () = assert!(...)` the report asked for now compiles. §2. An allocating ISR wedged the firmware in an unbounded spin. On a target built `--cfg ra_single_threaded` a lock observed held can only be reentrancy, so it is now a panic that names the ISR and spells out that the cfg means single CONTEXT. The proposed trigger would have misfired: `compare_exchange_weak` may fail SPURIOUSLY, so panicking on a failed CAS would abort firmwares at random -- worse than the hang. The shipped version confirms with a load first. Watched firing under `--cfg ra_single_threaded`, which CI now runs; deliberately not in gate-selftest.sh, because poisoning it hangs rather than fails. §3. `usable_bytes(base, len)` is public and const. One part of the review of this item was WRONG and is not implemented: `region_stats`' `free` was said to misreport the stranded tail, but that tail is genuinely available to page-sized allocations, so `free` is honest and the missing number was a different one. The backend's own tests exercise it as an extent allocator on a 512 KiB region, which cannot hold a 32 MiB segment. Rather than weaken the check or lose the coverage, the install half is split into a private `install_region` and the test branches on the active geometry. The refusal has no public bypass. Gates: 110/33 default, 91 small profile, prim::fixed under ra_single_threaded, clippy on three configs, both RISC-V targets at both geometries, wasm32, census, gate selftest 5/5, wasm size ratchet. Board re-flashed at 68 KiB: still accepted, used 69632 free 0, kill test green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Four findings from the first outside firmware to put 2.0.0 on a chip (
docs/plans/embedded-adoption.md). All verified against the code before anything changed.ra_small_profiledocumented nowhere, while68 KiBis quoted 5× as the floorinit_regionaccepts a region that can never yield a segmentusable_bytes()addedTwo proposals were changed on implementation, because the proposed form would not have worked.
§1's suggested
len < FIXED_PAGE + SEGMENT_SIZEis necessary but not sufficient — a segment can only start on a segment boundary, so a region passing that test with a misaligned base still yields nothing.init_regionhas the base, so it asks the exact question.§2's suggested trigger panics on a failed
compare_exchange_weak, which is permitted to fail spuriously — that would abort firmwares at random, worse than the hang it replaces. The shipped version confirms with a load first.And one part of my own review of §3 was wrong: I said
region_stats'freemisreports the stranded tail. It doesn't — that tail is genuinely available to page-sized allocations. The missing number was a different one, which is whatusable_bytesis.Gates: 110/33 default, 91 small profile,
prim::fixedunder--cfg ra_single_threaded, clippy on three configs, both RISC-V targets at both geometries, wasm32, unsafe census, gate selftest 5/5, wasm size ratchet. Board re-flashed at 68 KiB — still accepted,used 69632 free 0, kill test green.🤖 Generated with Claude Code