diff --git a/docs/architecture/schfs.md b/docs/architecture/schfs.md index db312b5..c1ea7c3 100644 --- a/docs/architecture/schfs.md +++ b/docs/architecture/schfs.md @@ -244,6 +244,27 @@ current code refuses and says why. --- +## Records are packed, and the compiler is told + +A leaf's value area holds variable-length records end to end, so a record starts +wherever the previous one finished — 8-aligned only by luck. The seven types +that live there carry `SCHFS_PACKED`, because casting a pointer into a block and +calling it a `struct schfs_dirent_rec *` is otherwise a lie to the compiler: the +type promises alignment the address does not have. + +**The annotation changes no on-disk layout.** Every one of those structs already +carries explicit `pad` fields and so has no implicit padding; sizes and offsets +are identical with and without it, and seven `_Static_assert`s hold them there. +What changes is the generated code — byte-wise access where alignment cannot be +proven, which is the truth of the situation. + +The block-level structures are deliberately excluded. `schfs_node_hdr` is +exactly 32 bytes, so the items and key pointers after it are already aligned, +and packing them would cost byte-wise access on the tree's hottest paths for +nothing. + +--- + ## One source, two compilers Everything except `kmount.c` is compiled **both** into the kernel and into the diff --git a/docs/guides/testing.md b/docs/guides/testing.md index bf6f784..759c78f 100644 --- a/docs/guides/testing.md +++ b/docs/guides/testing.md @@ -121,20 +121,27 @@ linked into the kernel binary for a `ramroot` build. --- -## One known failure left +## The suite is green -`make schfs-check` passes. UndefinedBehaviorSanitizer still reports **31 -misaligned reads** from casting pointers into tree leaves where records are not -8-aligned — itemised in [../plan/1.0.md](../plan/1.0.md), and printed on stderr -without failing the run. +`make schfs-check` passes with no sanitizer output at all. It was not, until +recently, and both failures are worth knowing about because neither was in the +filesystem's logic: -The leak half of this is closed, and how it closed is the useful part: the test -leaked 518 pages because the allocation map had no release path *at all*, which -meant the kernel leaked the same map on every unmount. **A suite that leaks -cannot detect a leak** — the filesystem's own allocations would have arrived as -noise inside the harness's. `schfs_objtest` now asserts that every page it -handed out came back, so the next one fails the suite rather than joining the -background. +**A leak with no API to fix it.** The test leaked 518 pages because the +allocation map had no release path *at all* — which meant the kernel leaked the +same map on every unmount. **A suite that leaks cannot detect a leak**: the +filesystem's own allocations would have arrived as noise inside the harness's. +`schfs_objtest` now asserts that every page it handed out came back. + +**31 misaligned reads.** Records are read by casting a pointer into a tree leaf, +where they sit at arbitrary offsets. That works on x86 and is undefined in C, so +it had never shown as a bug and would have arrived as apparent filesystem +corruption at some future optimisation level. + +The shared lesson is the one this directory keeps relearning: **a sanitizer that +is reliably noisy is a sanitizer nobody reads.** Both were dismissible as "known +and harmless" for as long as they were there, and one of them was a real kernel +leak. ## What is not tested diff --git a/docs/plan/1.0.md b/docs/plan/1.0.md index a04a7c7..9fdb1b0 100644 --- a/docs/plan/1.0.md +++ b/docs/plan/1.0.md @@ -164,25 +164,34 @@ known reasons stops being read, and then it is red for unknown reasons too. asserts the balance — 520 pages taken, 520 given back — so the next leak fails the suite instead of being absorbed by it. -- [ ] **31 UndefinedBehaviorSanitizer errors**, all one root cause: records are - read straight out of a tree leaf by casting a pointer into the block, and - a record's offset within a leaf is not a multiple of 8. Sites: - `obj.c` (9), `objw.c` (11), `attr.c` (7), `space.c` (5), across - `schfs_dirent_rec`, `schfs_extent`, `schfs_extref`, `schfs_index_rec` and - `schfs_snap`. +- [x] ~~**31 UndefinedBehaviorSanitizer errors.**~~ Fixed, plus two the test + never reached. One root cause: a leaf's value area holds variable-length + records packed end to end, so a record begins wherever the last one ended + — a multiple of 8 only by luck — and the code read them by casting a + pointer into the block, which tells the compiler they are aligned when + they are not. - **This is a real hazard, not sanitizer pedantry.** Unaligned access - happens to work on x86, which is why it has never shown as a bug — but it - is undefined in C, and a compiler that knows a pointer is 8-aligned is - entitled to emit an instruction that requires it. The failure would appear - under a different optimisation level or on a different architecture, and - would look like filesystem corruption rather than a codegen problem. + The seven record types that live at arbitrary offsets now carry + `SCHFS_PACKED`. **No on-disk layout changed**, and that was measured + before it was applied rather than assumed: every one already had explicit + `pad` fields, so sizes and member offsets are identical with and without + the annotation. Seven `_Static_assert`s now pin those sizes, because a + record that silently grows misreads every existing volume from that point + on and looks like corruption rather than like a struct that changed. - The fix that keeps the format unchanged is to stop casting: copy the bytes - into an aligned local with `memmove`, or declare the records packed so the - compiler stops assuming. Padding the records to 8 bytes would also work - and costs on-disk space — and is now a **format change**, which is what - the version policy above is for. + `schfs_node_hdr`, `schfs_item`, `schfs_kptr` and `schfs_uberblock` are + deliberately left alone — the node header is exactly 32 bytes, so + everything after it is already 8-aligned, and annotating them would cost + byte-wise access on the tree's hottest paths to fix a problem they do not + have. + + Verified by reading an image written *before* the change: full + `schfs_fsck --check` passes on structure, checksums, key ordering, data + blocks and derived free space. + + Also caught on the way: `mkschfs` passed a null pointer to `qsort` for an + empty directory. Every implementation ignores it when the count is zero; + the standard does not say they have to. **The identity item:** diff --git a/include/schfs.h b/include/schfs.h index 25b5928..21097ce 100644 --- a/include/schfs.h +++ b/include/schfs.h @@ -13,6 +13,36 @@ #include +// --------------------------------------------------------------------------- +// Records that live at an arbitrary offset inside a block. +// +// A leaf's value area holds variable-length records packed end to end, so a +// record begins wherever the one before it ended -- which is a multiple of 8 +// only by luck. The code reads them by casting a pointer into the block, and +// that is a lie to the compiler unless it is told: the type says "8-byte +// aligned" and the pointer is not. +// +// It works on x86, which is why it never showed as a bug, and it is undefined +// in C. A compiler entitled to assume alignment may emit an instruction that +// requires it, and the failure would arrive at a different optimisation level +// or on a different architecture looking like filesystem corruption rather than +// like codegen. UndefinedBehaviorSanitizer reported 31 of these across obj.c, +// objw.c, attr.c and space.c. +// +// This annotation changes NO on-disk layout. Every struct it is applied to +// already carries explicit `pad` fields and therefore has no implicit padding; +// sizes and member offsets are identical with and without it, which was +// measured rather than assumed before it was added. What it changes is the +// code generated to read them: byte-wise where the compiler cannot prove +// alignment, which is exactly the truth here. +// +// It is deliberately NOT applied to schfs_node_hdr, schfs_item, schfs_kptr or +// schfs_uberblock. Those sit at fixed offsets in a block -- the header is 32 +// bytes, so everything after it is 8-aligned -- and annotating them would cost +// byte-wise access on the hottest paths in the tree to fix a problem they do +// not have. +#define SCHFS_PACKED __attribute__((packed)) + #define SCHFS_MAGIC 0x53636846u // "SchF" #define SCHFS_NODE_MAGIC 0x536e6f64u // "Snod" @@ -208,7 +238,7 @@ struct schfs_snap { uint32_t pad0; uint64_t created; // generation the snapshot was taken at char name[SCHFS_SNAP_NAME_MAX]; -}; +} SCHFS_PACKED; struct schfs_uberblock { uint32_t magic; // SCHFS_MAGIC @@ -283,7 +313,7 @@ struct schfs_inode { uint32_t flags; // SCHFS_I_* — none defined yet uint32_t gen; // transaction that last changed this inode; // what an incremental send/snapshot diff needs -}; +} SCHFS_PACKED; // An extent maps a run of file blocks onto a run of disk blocks. refs is what // makes snapshots and reflink copies nearly free: sharing a range is a refcount @@ -313,14 +343,14 @@ struct schfs_extent { // without consulting the tree at all. uint32_t refs; uint32_t pad; -}; +} SCHFS_PACKED; // How many files point at a run of blocks. Present only for runs with more than // one reference; see SCHFS_EXTREF_OBJECTID. struct schfs_extref { uint64_t nblocks; uint64_t refs; -}; +} SCHFS_PACKED; // Directory entry payload. The key's offset field holds the name hash, so a // lookup is one tree descent rather than a directory scan. @@ -337,7 +367,7 @@ struct schfs_dirent_rec { uint16_t namelen; uint32_t pad; /* name bytes follow, namelen of them, not NUL-terminated */ -}; +} SCHFS_PACKED; #define SCHFS_NAME_MAX 255 @@ -364,7 +394,7 @@ struct schfs_attr_rec { uint16_t flags; uint32_t pad; /* name bytes, then value bytes; the pair padded to an 8-byte boundary */ -}; +} SCHFS_PACKED; // What a value *is*, so that a query can compare two of them meaningfully. A // string and an integer that happen to share bytes are not the same value, and @@ -392,7 +422,7 @@ struct schfs_index_rec { uint32_t valuelen; uint32_t pad; /* value bytes follow, so a query can reject a hash collision */ -}; +} SCHFS_PACKED; #define SCHFS_INDEX_LEN(valuelen) \ ((unsigned)((sizeof(struct schfs_index_rec) + (valuelen) + 7) & ~7u)) @@ -437,3 +467,14 @@ SCHFS_ASSERT_SIZE(schfs_attr_rec, 16); SCHFS_ASSERT_SIZE(schfs_index_rec, 16); #endif /* SCHFS_H */ + +// The annotation above must not have moved anything. These are on-disk sizes: +// if one changes, every existing volume is misread from that record onward, and +// the failure looks like corruption rather than like a struct that grew. +_Static_assert(sizeof(struct schfs_dirent_rec) == 16, "dirent record size changed"); +_Static_assert(sizeof(struct schfs_extent) == 32, "extent record size changed"); +_Static_assert(sizeof(struct schfs_extref) == 16, "extref record size changed"); +_Static_assert(sizeof(struct schfs_index_rec) == 16, "index record size changed"); +_Static_assert(sizeof(struct schfs_attr_rec) == 16, "attr record size changed"); +_Static_assert(sizeof(struct schfs_snap) == 96, "snapshot record size changed"); +_Static_assert(sizeof(struct schfs_inode) == 80, "inode record size changed"); diff --git a/tools/mkschfs.c b/tools/mkschfs.c index 5fcdbbd..6a16543 100644 --- a/tools/mkschfs.c +++ b/tools/mkschfs.c @@ -584,7 +584,11 @@ add_tree(uint64_t dirino, const char *hostdir) } closedir(d); - qsort(l.v, l.n, sizeof(*l.v), nl_cmp); + // qsort declares its first argument non-null, and an empty directory + // leaves l.v at NULL. Every implementation ignores the pointer when the + // count is zero; the standard does not say they have to. + if (l.n > 0) + qsort(l.v, l.n, sizeof(*l.v), nl_cmp); long subdirs = 0; for (int i = 0; i < l.n; i++) { subdirs += add_entry(dirino, hostdir, l.v[i]);