schfs: the allocation map can be given back, so unmount stops leaking it
Started as "fix the ASAN leak in schfs_objtest" and the test turned out to be
the messenger. The allocation map was taken through a palloc callback and there
was no way to return it: 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.
Three separate leaks from that one gap:
* The kernel leaked the entire map on every unmount -- up to 2 MB, at 8 KiB
per GiB of volume -- because kmount_unmount flushed, cleared `mounted` and
walked away. This is the one that matters; the others are test-only.
* schfs_space_init stranded a page on its own error path. npages was raised
only after *both* of a pair of allocations succeeded, so a failure on the
second left the first outside the count -- unreachable to any cleanup that
could later exist.
* schfs_objtest leaked 2121728 bytes in 518 allocations, which is what made
the whole SchFS suite exit non-zero regardless of its assertions.
The map now carries its own pfree rather than taking one at release time, so it
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. schfs_vol_open and schfs_vol_format release any map the volume
is already holding before building another, which is what stops a re-open
leaking; struct schfs_vol must therefore be zeroed before first use, and the
header now says so.
The test's own leak was the last one found and the simplest: it does
memset(&VOL, 0, sizeof(VOL)) before reopening, which wiped the map's pointers
before vol_open could release them. Eight of those, each now releasing first.
schfs_objtest asserts the balance -- 520 pages taken, 520 given back -- because
a suite that leaks cannot detect a leak: the filesystem's own allocations would
arrive as noise inside the harness's. The next one fails the run instead.
make schfs-check now exits 0. The 31 UBSAN misalignment reports are still
there and are still a Gate 0 item.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQ7Qx8E1q1M1bbCq8QLHS2
This commit is contained in:
parent
3e2f3cd1ec
commit
25530e7eed
9 changed files with 175 additions and 57 deletions
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue