diff --git a/docs/guides/testing.md b/docs/guides/testing.md index 9f2475b..bf6f784 100644 --- a/docs/guides/testing.md +++ b/docs/guides/testing.md @@ -121,21 +121,20 @@ linked into the kernel binary for a `ramroot` build. --- -## The suite is currently red +## One known failure left -`make schfs-check` **exits non-zero at baseline**, and has for some time. Two -separate causes, both in the harness rather than the filesystem, both itemised -in [../plan/1.0.md](../plan/1.0.md): +`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. -- AddressSanitizer reports 2121728 bytes leaked in 518 allocations, all from the - test's own page allocator, which never frees. -- UndefinedBehaviorSanitizer reports 31 misaligned reads, from casting pointers - into tree leaves where records are not 8-aligned. - -Worth knowing before you run it, and worth fixing rather than living with: the -assertions still pass, so it is tempting to read the `[ok]` lines and ignore the -exit code — which is exactly how a suite stops being a check and becomes a -decoration. +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. ## What is not tested diff --git a/docs/plan/1.0.md b/docs/plan/1.0.md index c4f6209..a04a7c7 100644 --- a/docs/plan/1.0.md +++ b/docs/plan/1.0.md @@ -146,18 +146,23 @@ it, and both predate the current work — confirmed by stashing and re-running. They are here rather than filed as nice-to-haves because a suite that is red for known reasons stops being read, and then it is red for unknown reasons too. -- [ ] **`make schfs-check` fails at baseline.** AddressSanitizer reports - **2121728 bytes leaked in 518 allocations**, every one of them from - `md_palloc` (`tools/schfs_objtest.c:84`) — the test's own page allocator, - which hands out pages for the allocation map and never frees them. The - target exits non-zero, so the *whole* SchFS suite is failing whatever the - assertions say. +- [x] ~~**`make schfs-check` fails at baseline.**~~ Fixed, and it was not only a + test bug. The allocation map was taken through a `palloc` callback and + there was **no way to give it back** — no `schfs_space_release`, and a + comment in `schfs_space_init` reading *"caller frees what was taken"* for + an API the caller did not have. - The cost is not the leaked memory, it is that **the leak check is now - useless**: a real leak in the filesystem would arrive as noise inside - 2 MB of expected noise. Fixing it means the test freeing what it - allocated, not suppressing the report — a suppression keeps the number - quiet and keeps the detector off. + So the kernel leaked the whole map on **every unmount**, up to 2 MB a + time, and `schfs_space_init`'s own error path stranded a page it had + already taken because `npages` was only raised after *both* allocations + succeeded. The test's 518 leaked pages were the symptom that pointed at + it. + + The map now carries its own `pfree` so it can only be freed by the + allocator that filled it, `schfs_vol_open`/`schfs_vol_format` release any + map the volume already holds, and unmount releases. `schfs_objtest` + 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 diff --git a/kernel/fs/schfs/kmount.c b/kernel/fs/schfs/kmount.c index fd83518..8d587cb 100644 --- a/kernel/fs/schfs/kmount.c +++ b/kernel/fs/schfs/kmount.c @@ -447,6 +447,15 @@ kd_palloc(void) return kalloc(); } +// The other half of kd_palloc, and the reason unmount stopped leaking: the +// allocation map is up to SCHFS_SPACE_PAGES pages of kalloc'd memory, and +// nothing ever gave it back. A mount/unmount cycle leaked the lot. +static void +kd_pfree(void *p) +{ + kfree((char *)p); +} + // Mount the SchFS volume on block device `bdev` into slot `slot`. // // Never panics: a disk that is not SchFS, or is an unreadable SchFS, simply @@ -495,7 +504,7 @@ kmount_slot(int slot, uint bdev) // Opening with a page allocator also builds the allocation bitmap, by walking // the tree — the only mount-time cost SchFS has, and what buys it a mount // that cannot disagree with the filesystem it just adopted. - int r = schfs_vol_open(&m->vol, &bd, &cx, kd_palloc); + int r = schfs_vol_open(&m->vol, &bd, &cx, kd_palloc, kd_pfree); if (r == -1) return -EINVAL; /* not a SchFS volume at all */ if (r == -4) { @@ -594,6 +603,10 @@ schfs_kumount(uint dev) acquiresleep(&m->lock); bflush(m->kdev.dev); + // Give the allocation map back. It is rebuilt from the tree on every mount, + // so there is nothing in it worth keeping -- and at 8 KiB per GiB of volume, + // keeping it was the largest thing this kernel leaked. + schfs_space_release(&m->vol.space); m->mounted = 0; releasesleep(&m->lock); return 0; diff --git a/kernel/fs/schfs/schfs_int.h b/kernel/fs/schfs/schfs_int.h index ae8345e..61d690b 100644 --- a/kernel/fs/schfs/schfs_int.h +++ b/kernel/fs/schfs/schfs_int.h @@ -45,10 +45,22 @@ struct schfs_space { uint64_t hint; // rotating search start // SCHFS_SPACE_RETAIN_ALL while a snapshot exists -- see schfs_space_free. unsigned flags; + // How to give the pages back. Carried here rather than passed to the + // release call so that a map can only ever be freed by the allocator that + // filled it: the kernel's pages come from kalloc and a host tool's from + // calloc, and the two are not interchangeable. + void (*pfree)(void *); }; // A mounted (or under-construction) volume. // +// MUST BE ZEROED before its first use. schfs_vol_open and schfs_vol_format both +// release any allocation map the volume is already holding before building a +// new one -- that is what stops a re-open leaking the old map -- and on a first +// use they are reading whatever the struct was born with. Every volume in the +// tree is static storage, so this costs nothing; a stack-allocated one would +// have to be memset first. +// // NOT copyable: tree.dev points at this struct's own dev field, so a copy // leaves the tree talking to whichever struct it was built in. Open into the // storage it will live in. @@ -112,12 +124,14 @@ struct schfs_vol { // yields pages for the allocation bitmap and may be NULL for a read-only mount, // which then cannot allocate at all. int schfs_vol_open(struct schfs_vol *v, struct schfs_bdev *dev, - struct schfs_ctx *x, void *(*palloc)(void)); + struct schfs_ctx *x, void *(*palloc)(void), + void (*pfree)(void *)); // Lay down an empty volume: one empty tree and one uberblock. int schfs_vol_format(struct schfs_vol *v, struct schfs_bdev *dev, uint64_t total_blocks, const char *label, - struct schfs_ctx *x, void *(*palloc)(void)); + struct schfs_ctx *x, void *(*palloc)(void), + void (*pfree)(void *)); // Publish the current tree by writing the next slot in the ring. Until this // returns, everything written since the last commit is invisible and a crash @@ -152,7 +166,19 @@ void schfs_vol_free(struct schfs_vol *v, uint64_t blk); // volume is too small to hold anything, -4 if it is larger than this build can // map (see SCHFS_MAX_BLOCKS), -5 if palloc ran out. int schfs_space_init(struct schfs_space *s, uint64_t nblocks, - void *(*palloc)(void)); + void *(*palloc)(void), void (*pfree)(void *)); + +// Hand every page back and zero the map. Safe on a map that was never +// initialised and safe to call twice, so an error path need not work out how +// far initialisation got. +// +// This exists because there was no way to give the pages back at all. The +// kernel leaked the whole map on every unmount -- up to 2 MB a time -- and +// schfs_objtest leaked one map per volume it opened, which ran AddressSanitizer +// up to 2 MB across 518 allocations and made the whole SchFS suite exit +// non-zero. A real leak in the filesystem would have arrived as noise inside +// that. +void schfs_space_release(struct schfs_space *s); // Return an already-sized map to "empty volume", reusing its pages. Mounting // may try several uberblocks, and each candidate root needs its scan to start diff --git a/kernel/fs/schfs/space.c b/kernel/fs/schfs/space.c index f3a431f..d45a615 100644 --- a/kernel/fs/schfs/space.c +++ b/kernel/fs/schfs/space.c @@ -126,11 +126,26 @@ schfs_space_reset(struct schfs_space *s, uint64_t nblocks) bit_set(s->inuse, b); } +void +schfs_space_release(struct schfs_space *s) +{ + if (s->pfree) { + for (int i = 0; i < s->npages; i++) { + if (s->inuse[i]) s->pfree(s->inuse[i]); + if (s->pend[i]) s->pfree(s->pend[i]); + } + } + // Zeroed rather than merely emptied, so calling this twice is harmless and + // a released map cannot be mistaken for a sized one. + memset(s, 0, sizeof(*s)); +} + int schfs_space_init(struct schfs_space *s, uint64_t nblocks, - void *(*palloc)(void)) + void *(*palloc)(void), void (*pfree)(void *)) { memset(s, 0, sizeof(*s)); + s->pfree = pfree; if (nblocks <= SCHFS_FIRST_BLOCK) return -3; @@ -141,16 +156,26 @@ schfs_space_init(struct schfs_space *s, uint64_t nblocks, / SCHFS_SPACE_BITS_PER_PAGE; for (uint64_t i = 0; i < npages; i++) { + // npages is raised as soon as inuse[i] is ours, not after both pages + // are. It used to be raised after, which meant a failure on the second + // allocation left the first one owned by nobody: outside the count, so + // unreachable to any cleanup, and never freed. The comment here said + // "caller frees what was taken" and there was nothing the caller could + // call. void *p = palloc(); - if (p == 0) - return -5; /* caller frees what was taken */ + if (p == 0) { + schfs_space_release(s); + return -5; + } s->inuse[i] = (uint64_t *)p; + s->npages = (int)(i + 1); p = palloc(); - if (p == 0) + if (p == 0) { + schfs_space_release(s); return -5; + } s->pend[i] = (uint64_t *)p; - s->npages = (int)(i + 1); } schfs_space_reset(s, nblocks); diff --git a/kernel/fs/schfs/vol.c b/kernel/fs/schfs/vol.c index cba4c9f..8e6f395 100644 --- a/kernel/fs/schfs/vol.c +++ b/kernel/fs/schfs/vol.c @@ -124,7 +124,8 @@ vol_from_uber(struct schfs_vol *v, struct schfs_uberblock *u, int slot) int schfs_vol_open(struct schfs_vol *v, struct schfs_bdev *dev, - struct schfs_ctx *x, void *(*palloc)(void)) + struct schfs_ctx *x, void *(*palloc)(void), + void (*pfree)(void *)) { struct schfs_uberblock *u = (struct schfs_uberblock *)x->a; int io_errors = 0; @@ -238,7 +239,16 @@ schfs_vol_open(struct schfs_vol *v, struct schfs_bdev *dev, // Sizing failures are the caller's problem, not this root's: // a volume too large to map or a machine out of pages will // fail identically on every slot in the ring. - int sr = schfs_space_init(&v->space, v->total_blocks, palloc); + // A volume that has been opened before still holds the + // previous map, and schfs_space_init memsets it away -- so + // without this every re-open leaks one. schfs_objtest reopens + // the same volume nine times. + // + // Safe on a first open because a struct schfs_vol must be + // zeroed before first use, which is what release reads: a + // zeroed map has no pfree and no pages, so it does nothing. + schfs_space_release(&v->space); + int sr = schfs_space_init(&v->space, v->total_blocks, palloc, pfree); if (sr != 0) return sr; spaced = 1; @@ -350,11 +360,16 @@ schfs_vol_commit(struct schfs_vol *v) int schfs_vol_format(struct schfs_vol *v, struct schfs_bdev *dev, uint64_t total_blocks, const char *label, - struct schfs_ctx *x, void *(*palloc)(void)) + struct schfs_ctx *x, void *(*palloc)(void), + void (*pfree)(void *)) { if (total_blocks <= SCHFS_FIRST_BLOCK) return -3; + // Same reasoning as the release in schfs_vol_open: formatting over a + // volume that was previously open would otherwise drop its map on the + // floor, and the memset below is what makes it unrecoverable. + schfs_space_release(&v->space); memset(v, 0, sizeof(*v)); v->dev = *dev; v->ctx = *x; @@ -365,7 +380,7 @@ schfs_vol_format(struct schfs_vol *v, struct schfs_bdev *dev, // Nothing is allocated on a fresh volume beyond the boot block and the // ring, so the map starts empty rather than being scanned into existence. { - int sr = schfs_space_init(&v->space, total_blocks, palloc); + int sr = schfs_space_init(&v->space, total_blocks, palloc, pfree); if (sr != 0) return sr; } diff --git a/tools/mkschfs.c b/tools/mkschfs.c index f56d74e..5fcdbbd 100644 --- a/tools/mkschfs.c +++ b/tools/mkschfs.c @@ -168,6 +168,12 @@ img_palloc(void) return calloc(1, SCHFS_BLKSIZE); } +static void +img_pfree(void *p) +{ + free(p); +} + /* ------------------------------------------------------------------ */ /* Content dedup */ /* */ @@ -688,7 +694,7 @@ main(int argc, char **argv) bd.balloc_run = img_alloc_run; bd.bflush = img_flush; - int r = schfs_vol_format(&VOL, &bd, nblocks, label, &CTX, img_palloc); + int r = schfs_vol_format(&VOL, &bd, nblocks, label, &CTX, img_palloc, img_pfree); if (r != 0) die("formatting", r); // The root directory is an object like any other; nothing points at it diff --git a/tools/schfs_fsck.c b/tools/schfs_fsck.c index 2f1a671..b6e3258 100644 --- a/tools/schfs_fsck.c +++ b/tools/schfs_fsck.c @@ -113,6 +113,7 @@ static void img_nofree(void *ctx, uint64_t b) { (void)ctx; (void)b; } // Pages for the allocation map --check derives. Nothing is written back; this // is a check on the map, not a use of it. static void *img_palloc(void) { return calloc(1, SCHFS_BLKSIZE); } +static void img_pfree(void *p) { free(p); } static struct schfs_vol VOL; static char CURBLK[SCHFS_BLKSIZE]; @@ -367,7 +368,7 @@ do_check(void) // stored, so nothing else would ever notice if the scan and the writer // disagreed — right up until a mounted volume handed out a block that was // still in use. - if (schfs_space_init(&VOL.space, VOL.total_blocks, img_palloc) != 0) { + if (schfs_space_init(&VOL.space, VOL.total_blocks, img_palloc, img_pfree) != 0) { fprintf(stderr, " FAIL: could not build an allocation map\n"); return 1; } @@ -785,7 +786,7 @@ main(int argc, char **argv) // No page allocator: this tool never writes, and a volume without an // allocation map cannot allocate even by accident. - int r = schfs_vol_open(&VOL, &bd, &CTX, NULL); + int r = schfs_vol_open(&VOL, &bd, &CTX, NULL, NULL); if (r == -1) { fprintf(stderr, "schfs_fsck: %s: no valid uberblock -- not a SchFS " "volume, or never successfully written\n", imgpath); diff --git a/tools/schfs_objtest.c b/tools/schfs_objtest.c index 4575965..79e9411 100644 --- a/tools/schfs_objtest.c +++ b/tools/schfs_objtest.c @@ -81,7 +81,13 @@ md_write(void *ctx, uint64_t b, const void *buf) static uint64_t md_alloc(void *ctx) { (void)ctx; return schfs_vol_alloc(&VOL); } static uint64_t md_run(void *ctx, uint64_t n) { (void)ctx; return schfs_vol_alloc_run(&VOL, n); } static void md_free(void *ctx, uint64_t b) { (void)ctx; schfs_vol_free(&VOL, b); } -static void *md_palloc(void) { return calloc(1, SCHFS_BLKSIZE); } +static long md_pages_taken, md_pages_given; +static void *md_palloc(void) { md_pages_taken++; return calloc(1, SCHFS_BLKSIZE); } +// The matching half. Without it every volume this test opened leaked its whole +// allocation map -- 2121728 bytes across 518 allocations by the end, which made +// the target exit non-zero and drowned any real leak the filesystem might have +// had in expected noise. +static void md_pfree(void *p) { md_pages_given++; free(p); } static void mkbdev(struct schfs_bdev *bd) @@ -221,8 +227,9 @@ remount(void) // Straight back into VOL, not into a local that is then copied over it: a // schfs_vol points at its own dev field, so a copy of one leaves the tree // talking to whichever struct it was built in. - memset(&VOL, 0, sizeof(VOL)); - if (schfs_vol_open(&VOL, &bd2, &CTX, md_palloc) != 0) { + schfs_space_release(&VOL.space); /* before the memset strands it */ + memset(&VOL, 0, sizeof(VOL)); + if (schfs_vol_open(&VOL, &bd2, &CTX, md_palloc, md_pfree) != 0) { printf(" FAIL: remount\n"); failures++; return ~0ULL; @@ -273,7 +280,7 @@ main(void) printf("SchFS object-layer test\n\n"); - if (schfs_vol_format(&VOL, &bd, NBLOCKS, "objtest", &CTX, md_palloc) != 0) { + if (schfs_vol_format(&VOL, &bd, NBLOCKS, "objtest", &CTX, md_palloc, md_pfree) != 0) { fprintf(stderr, "FAIL: format\n"); return 1; } @@ -917,8 +924,9 @@ main(void) struct schfs_bdev bd3; mkbdev(&bd3); + schfs_space_release(&VOL.space); /* before the memset strands it */ memset(&VOL, 0, sizeof(VOL)); - int r = schfs_vol_open(&VOL, &bd3, &CTX, md_palloc); + int r = schfs_vol_open(&VOL, &bd3, &CTX, md_palloc, md_pfree); ok(r == 0, "the volume mounts after a crash mid-transaction"); ok(VOL.generation == good_gen, "it comes back at the last committed generation, not a partial one"); @@ -964,9 +972,10 @@ main(void) for (long cut = 1; cut < 120; cut++) { memcpy(DISK, saved, NBLOCKS * SCHFS_BLKSIZE); - memset(&VOL, 0, sizeof(VOL)); + schfs_space_release(&VOL.space); /* before the memset strands it */ + memset(&VOL, 0, sizeof(VOL)); mkbdev(&bd3); - if (schfs_vol_open(&VOL, &bd3, &CTX, md_palloc) != 0) { + if (schfs_vol_open(&VOL, &bd3, &CTX, md_palloc, md_pfree) != 0) { bad++; break; } @@ -994,9 +1003,10 @@ main(void) // Whatever the disk looks like now, all three of these must // hold: it mounts, it is a whole tree, and the data committed // before the crash is exactly as it was. - memset(&VOL, 0, sizeof(VOL)); + schfs_space_release(&VOL.space); /* before the memset strands it */ + memset(&VOL, 0, sizeof(VOL)); mkbdev(&bd3); - if (schfs_vol_open(&VOL, &bd3, &CTX, md_palloc) != 0) { + if (schfs_vol_open(&VOL, &bd3, &CTX, md_palloc, md_pfree) != 0) { printf(" cut %ld: does not mount\n", cut); bad++; continue; } @@ -1040,9 +1050,10 @@ main(void) // the root. Before the fallback existed the mount simply refused, with // 127 intact older roots sitting on the disk untouched. memcpy(DISK, saved, NBLOCKS * SCHFS_BLKSIZE); + schfs_space_release(&VOL.space); /* before the memset strands it */ memset(&VOL, 0, sizeof(VOL)); mkbdev(&bd3); - if (schfs_vol_open(&VOL, &bd3, &CTX, md_palloc) != 0) { + if (schfs_vol_open(&VOL, &bd3, &CTX, md_palloc, md_pfree) != 0) { printf(" FAIL: reopen for the misdirected-write case\n"); failures++; } else { @@ -1062,9 +1073,10 @@ main(void) for (int i = 0; i < 2; i++) memset(DISK + roots[i] * SCHFS_BLKSIZE, 0xA5, SCHFS_BLKSIZE); - memset(&VOL, 0, sizeof(VOL)); + schfs_space_release(&VOL.space); /* before the memset strands it */ + memset(&VOL, 0, sizeof(VOL)); mkbdev(&bd3); - int rr = schfs_vol_open(&VOL, &bd3, &CTX, md_palloc); + int rr = schfs_vol_open(&VOL, &bd3, &CTX, md_palloc, md_pfree); ok(rr == 0, "a volume whose two newest roots are rubbish still mounts"); // Strictly older, not "exactly two older". Copy-on-write shares // every unchanged node between generations, so a block that is part @@ -1088,19 +1100,35 @@ main(void) memcpy(DISK, saved, NBLOCKS * SCHFS_BLKSIZE); memset(DISK + SCHFS_UBER_START * SCHFS_BLKSIZE, 0, SCHFS_UBER_COUNT * SCHFS_BLKSIZE); + schfs_space_release(&VOL.space); /* before the memset strands it */ memset(&VOL, 0, sizeof(VOL)); mkbdev(&bd3); - ok(schfs_vol_open(&VOL, &bd3, &CTX, md_palloc) != 0, + ok(schfs_vol_open(&VOL, &bd3, &CTX, md_palloc, md_pfree) != 0, "a volume with no valid uberblock at all is refused, not invented"); // Put the good image back so anything after this runs on a sane volume. memcpy(DISK, saved, NBLOCKS * SCHFS_BLKSIZE); + schfs_space_release(&VOL.space); /* before the memset strands it */ memset(&VOL, 0, sizeof(VOL)); mkbdev(&bd3); - schfs_vol_open(&VOL, &bd3, &CTX, md_palloc); + schfs_vol_open(&VOL, &bd3, &CTX, md_palloc, md_pfree); free(saved); } + // The last volume opened is still holding its map. Nothing else will take + // it, and a test that leaks cannot detect a leak: the filesystem's own + // allocations would arrive as noise inside this one. + schfs_space_release(&VOL.space); + + if (md_pages_taken != md_pages_given) { + printf(" [FAIL] %ld allocation-map pages taken, %ld given back\n", + md_pages_taken, md_pages_given); + failures++; + } else { + printf(" [ok] every allocation-map page was given back (%ld)\n", + md_pages_taken); + } + printf("\n%s\n", failures ? "FAILED" : "all checks passed"); return failures ? 1 : 0; }