Repository navigation
Conversation
|
Thanks for putting this together, and for flagging the overlap with #234 up front. At +6,969/-128 across 47 files, it's more than I can review carefully as one PR, and SEP state code is exactly where I want to be careful. Could you split it into reviewable pieces, for example atomic host-file replacement, enrollment index publication, keybag refusal/intent records, and reference-key publication? It would also help to say which of those #234 already covers, so we only review what's new. I've approved #234, so if it lands first, please rebase on it. |
Prepare a complete file under an exclusive temporary name, fsync it, rename under the directory lock and fsync the parent before reporting success. Refuse non-regular destinations and keep visibility distinct from proven durability after rename. Shared primitives also support later exclusive-file and host-store header publication. Compile the production C and Rust paths in synthetic VFS fault tests with Rust 1.85.0. Fifteen catacomb cases and 33 C boundary cases pass; reverting the writer to truncate-in-place fails the named preservation assertions. No hardware or power-loss qualification is claimed. Signed-off-by: Matt <mmdisbro@gmail.com>
699e750 to
06902b3
Compare
|
Thanks, that makes sense. I reduced this PR to atomic publication only. The enrollment index, keybag and reference-key pieces are now separate comparison branches on my fork that I'll submit after this one, so I'm not adding them to this PR. I've documented which parts overlap with #234 and kept its patches with their original authors. I'll rebase onto the real merged head if #234 lands first. |
There was a problem hiding this comment.
The fix applies to every Mac and we'd like it in the release candidate: a crash can no longer truncate a catacomb. We'll approve once one enrollment on hardware shows the catacombs get replaced and no .sep-*.tmp files are left behind. Trimming the unused C below is a request, not a blocker.
Thanks for splitting this. We reviewed 06902b3 alone and merged with #234 (accecc6). It merges clean both ways, and the atomic suite passes on the merge too, so the order doesn't matter to us.
The replace path looks correct. The temp file is created O_EXCL|O_NOFOLLOW in the same directory (store_shim.c:214), written, fsynced, renamed with the destination checked under the rename lock, then the parent is fsynced (store_shim.c:263). If we crash before the rename, the old catacomb is intact. If we crash after the rename but before the parent fsync, we get either the old or the new file, never a torn one. A write error means save_catacomb() never sends CONFIRM_SAVE (sbio.rs:1007). Today a crash mid-write leaves a truncated file. That reads as "absent" now and as an error once #234 lands, so this is a real fix.
- store_shim.c:105, :276, :300, :308 and shim.h:47-51: You note the header and create wrappers come with their callers later. We'd rather the C side moved with them too, since nothing here calls or tests it: the fixture builds
sep_store_create, but no case reaches its-EEXISTdestination check, and the README's "exclusive-publication races" refers to temp-name collisions. The header path isn't reached either: the fixture stubsi_size_read,kmallocandkernel_readwithabort()(persistence-tests.py:156-158). Keeping onlysep_store_replacehere, withstore_replacereduced to what it uses, would make this easier to land. - Minor: a crash between create and rename leaves a
.sep-<hex>.tmpfile in /var/lib (up to 1 MiB) that nothing ever removes. That's fine for now, but a cleanup at probe, or a note in the README, would help.
What we ran: persistence-tests.py --suite atomic with Rust 1.85.0 passes (15 Rust cases, 33 C cases), and the truncate-in-place control fails 10 tests. It also passes on the #234 merge. We tried four mutants of our own: skipping the temp fsync, skipping the parent fsync, dropping the non-regular-destination check, and skipping temp cleanup. The C boundary tests caught all four. We didn't do a kernel build.
Keep the catacomb replacement primitive limited to full-file writes. Move header-tail copying and exclusive destination creation out of this change, trim their fixture stubs, and document temporary files left by a crash before rename. Signed-off-by: Matt <mmdisbro@gmail.com>
|
Thanks, Scott. I pushed c47d11b with the DCO sign-off. It removes the unused C header/exclusive-create paths and fixture stubs, and documents possible crash leftovers and temporary-name collisions. I completed three attended enrollments on a J313 using combined test build 39e8f218d7165d2c00d7d2c716b0975d4e47ff2d, kernel 7.2.11-9-1.251-ARCH. This was not standalone #250. Direct source comparison matched the catacomb writer and relevant helpers, allowing for the combined build's store_replace_full name. The loaded SEP build-ID note matched the packaged candidate. All three enrollments completed with saved exit code 0. Inotify recorded temporary CREATE and destination MOVED_TO events for user/master, and their destination inodes changed. Rename cookies were not captured, so individual temp-to-destination attribution also relies on event ordering and the inspected writer. The owner was also replaced when enrolling after I accidentally deleted all templates; it was unchanged when adding a finger to an existing context. The captured post-enrollment temp listings were empty; their command exit codes were not separately saved. After reboot, all three catacombs reported Restored with two identities, the loaded build-ID note still matched, and both fingers returned verify-match. Recorded inodes and sizes were unchanged across reboot; contents were not compared. Saved tests match the trimmed source: 15 Rust and 33 C cases passed, with the truncation control failing 10 cases. xART guard was off. These are normal enrollment and reboot results, not standalone-kernel, power-loss/crash, fresh-provisioning, anti-replay or guarded-xART qualification. The combined-build limitation remains for your assessment. Kernel CI is still awaiting maintainer approval. For one concrete replacement snapshot (round 1), user changed from inode 349595 / 11400 bytes to 1067067 / 24184; master from 349596 / 300 to 1067068 / 340; owner stayed at 349592 / 120. The loaded SEP GNU build ID was Could a maintainer approve the held Kernel CI run and apply |
A failed catacomb write currently truncates the committed host file before its replacement is complete. Publish the complete replacement through a temporary file instead: write, sync the file, rename under the directory lock, then sync the parent before reporting success.
This is the first, reduced piece of the former persistence proposal. It adds the shared C publication primitive and its Rust replacement wrapper, and uses replacement for catacomb writes. Header and exclusive-creation Rust wrappers accompany their callers in the later pieces. Header publication, enrollment-index completion, keybag writers and reference-key publication are separate follow-ups. It does not change reader error handling or xART policy.
The primitive rejects non-regular destinations, never adopts an existing temporary file, and distinguishes a visible rename from proven directory durability. A failed parent sync returns an error even though the complete replacement may already be visible.
Relationship to #234: that PR covers open-error preservation, complete identity-index records, canonical refused-keybag validation, catacomb error-vs-absence handling and the reference-key ENOENT gate. This piece builds on the current upstream independently and does not reimplement those reader changes. If #234 lands first, this will be rebased onto the actual merged upstream head.
Validation on this individual branch:
Rust 1.85.0 production-source fixture: 15 catacomb/VFS cases pass.
Production C publication boundary fixture: 33 cases pass.
The compiled truncate-in-place negative control fails the required previous-generation preservation assertion.
CI discovery/registration tests pass; the JSON diff adds only this suite.
Native SEP C and Rust object compilation uses the archived kernel headers and Rust 1.85.0. This is not a full kernel/package build or an upstream CI result.
The standalone PR kernel has not been qualified by enrollment. Recorded enrollment and reboot results on a combined J313 test build exercise matching catacomb writer functions, with xART guard off; the complete kernels differ. No physical power-loss, crash-durability or SEP anti-replay result is claimed. AI-assisted with OpenAI Codex; commits carry the contributor-authorized DCO sign-off.
Scoped follow-up comparisons (fork review branches; not upstream PRs):
Atomic foundation reconciled with #234
Host-store header publication
Enrollment-index publication
Keybag writer publication
Reference-key publication
These comparisons use an explicitly synthetic base made from current upstream plus only #234's three SEP commits. #234 is not merged. Each link shows only its parent-to-child piece; the later branch ancestry must not be submitted as a cumulative upstream PR. The inherited reader implementations and authorship are preserved.