Skip to content

cow: subtree-aware whiteouts with a durable deletion log; type-aware copy-up#162

Open
congwang-mk wants to merge 7 commits into
mainfrom
cow-deletion-subtree
Open

cow: subtree-aware whiteouts with a durable deletion log; type-aware copy-up#162
congwang-mk wants to merge 7 commits into
mainfrom
cow-deletion-subtree

Conversation

@congwang-mk

Copy link
Copy Markdown
Contributor

Redesign of the seccomp COW branch's deletion model. The flat exact-match HashSet<String> (inherited from the Python-era cowfs/_branch.py, carried verbatim into the Rust rewrite) is replaced by a subtree-aware whiteout set, and copy-up becomes file-type-aware.

Fixes #158
Fixes #159
Fixes #160
Fixes #161

The model

A new DeletionSet (cow/deletions.rs) owns whiteout semantics:

  • covers(rel) matches per path component: a whiteout on d hides d/x and d/x/y, never d2.
  • Entries are never removed. A path re-created after deletion is exposed by existing in the upper (upper presence shadows the whiteout), which makes rmdir d; mkdir d an opaque directory for free: the new d is visible, its old contents stay hidden.
  • Every insertion is appended to deleted.log beside upper/ and fdatasynced, so the deletion half of a change set is now self-describing on disk, like the upper already was. RAM stays the in-process source of truth; the log is the durability record a future recovery sweep (RFC RFC: Multi-sandbox transactions for pipelines #65 Phase 3, PR pipeline: transactional sequential pipelines over a shared COW workdir (RFC #65 Phase 1) #148's preserved branches) can replay.

All merged-view queries (is_deleted, open/stat/readlink, list_merged_dir, changes(), commit()) route through it, instead of each handler re-implementing its own slice of overlay semantics.

Per issue

The invariant

One test pins the property all four issues violated: the merged view the child observed during the run is exactly what commit() publishes. Plus each issue's own reproducer as a unit test, and three end-to-end regressions driving a real sandboxed child (rm -r, mv dir, rmdir non-empty).

Compatibility

No public API, CLI, or SDK signature changes. Child-visible behavior changes only toward kernel semantics (ENOTEMPTY, ENOENT under deleted dirs, dir renames preserving contents). The branch storage dir gains deleted.log; the layout is internal.

Suites: 555 lib / 308 integration / 386 Python locally; the two cli_test failures on my machine (test_learn_captures_openat2, test_write_collapse_skips_protected) reproduce identically on main and are unrelated.

Signed-off-by: Cong Wang <cwang@multikernel.io>
…visibility and resurrection (#159)

Signed-off-by: Cong Wang <cwang@multikernel.io>
Signed-off-by: Cong Wang <cwang@multikernel.io>
Signed-off-by: Cong Wang <cwang@multikernel.io>
…sions

Signed-off-by: Cong Wang <cwang@multikernel.io>
…d of passing through (#158)

Signed-off-by: Cong Wang <cwang@multikernel.io>
@dzerik

dzerik commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Thanks for taking on the whole deletion model — the subtree-aware whiteouts and the merged-view rmdir/ENOTEMPTY are a real improvement. From the reporter side I can confirm #161 is fully closed (opaque dir after rmdir d; mkdir d), #159's core is closed (reads, listings and O_CREAT under a deleted dir all ENOENT/start-fresh — the O_CREAT-no-resurrect bonus holds), and #160's reported case is closed (a renamed dir keeps its children, whiteouted children do not reappear, a whiteouted source gives ENOENT). Type-aware copy-up does close the copy-up-time hang.

I re-ran each of the four as an in-cage test against this branch. A few things I would want resolved before it lands.

Blocker — #158 is only half-closed; the hang is relocated to commit().

prepare_copy (cow/seccomp.rs:288-306) virtualizes a FIFO/socket/device as an empty regular upper stub but never mark_deleteds the surviving lower special file. commit() applies deletions first (943-950 — the FIFO is not in the set), then walks the upper, sees the stub as a regular file, and at 989-995 does openat2_in_root(workdir, rel, O_WRONLY|O_CREAT|O_TRUNC|O_NOFOLLOW). That destination is the surviving lower FIFO: O_NOFOLLOW does not affect FIFOs and openat2_in_root adds no O_NONBLOCK, so an O_WRONLY open of a reader-less FIFO blocks forever. commit() runs in impl Drop with the error swallowed, so this is an unrecoverable supervisor hang, not a failed merge.

Repro: lower FIFO pipe, child does echo hi > pipe, BranchAction::Commit — the commit blocks (>25s watchdog, pipe still a FIFO on disk). The identical child with Abort passes, which isolates it to the commit publish path. The existing FIFO tests (copy_up_of_fifo, write_open_of_fifo, chmod_of_fifo) all stop before commit(), so CI stays green.

Fix options: mark_deleted the lower path when stubbing a non-regular entry (commit then unlinks-then-creates), unlink a type-mismatched destination before opening, or open the destination O_NONBLOCK and handle ENXIO.

Should-fix — a directory rename onto an existing lower-only destination silently merges the two trees (new, reachable via #160).

handle_rename (seccomp.rs:688-699) stages the source into the upper with copy_up_tree and then renameat_in_root(upper, old, new). When new exists only in the lower it is never whiteouted, so the merged view of new becomes the union of the renamed tree and the pre-existing lower tree, and the syscall returns success. With lower-only a/{a1} and b/{b1}, mv -T a b returns 0 and commits b = {a1, b1} — a state that never existed. POSIX wants ENOTEMPTY for a non-empty destination, or an atomic replace.

Should-fix — #159 gap: the O_DIRECTORY open path never consults is_deleted/covers.

handle_open returns Ok(None) for O_DIRECTORY unconditionally (450-452), and prepare_open's O_DIRECTORY branch resolves to the upper only when upper_dir.is_dir() && !lower_dir.is_dir() (519), otherwise SkipContinue → the kernel opens the lower directory. After a normal rm -rf d of a lower-only dir, stat("d") gives ENOENT but open("d", O_DIRECTORY) succeeds on the lower fd, and fstat on it leaks the deleted directory's inode/nlink/mtime. The blast radius is bounded (getdents is virtualized empty via list_merged_dir, and openat(dirfd, child) is re-gated through absolute-path reconstruction), but an open succeeding where the merged answer is ENOENT breaks the "present as absent" invariant this PR pins elsewhere. Short-circuiting is_deleted(rel) before the O_DIRECTORY split in both functions closes it.

Should-fix — copy_up_tree failure paths (seccomp.rs:402-426).

read_dir/statat errors are swallowed as Ok(()) (406, 416), so a mid-tree EACCES/EIO truncates the staged copy — and handle_rename then mark_deleteds the source anyway (697), so the untraversed children are silently lost at commit. It also recurses with no depth bound over child-controlled directory depth (a pre-seeded deep lower tree can overflow the supervisor's stack), and a mid-tree QuotaExceeded leaves disk_used charged and an orphan upper subtree behind with no rollback. Propagating those errors and making the staging all-or-nothing would match the guarantees the rest of the branch gives.

Nice-to-have — deleted.log format and durability (latent while load() is dead_code).

escape() (deletions.rs:27-37) escapes \\ and \n but not \r, while load() reads via BufRead::lines(), which strips a trailing \r. A file named "d/file\r" round-trips to "d/file", after which covers() hides the wrong sibling and the actually-deleted path resurrects. Separately, create() never fsyncs the parent branch_dir and insert() only sync_data()s the log file, so a power loss after the first deletion can lose the log's directory entry entirely — the exact crash class the log exists for. Escaping \r (or length-delimiting) and one fsync of branch_dir after create would close both.

Coupling with #148 — worth deciding the sequencing before either merges.

These two rework the same SeccompCowBranch commit/changes/deletion surface, and they collide semantically rather than textually:

Suggested order: land DeletionSet first, then rebuild #148's preserve/marker/remainder logic on a single deletion source of truth — either dropping the log's recovery role or having preserve() snapshot DeletionSet::iter() into the marker. Happy to drive that on the #148 side.

Only the #158 commit hang is a hard blocker for me; the rest is should-fix/nice-to-have plus the sequencing call.

@dzerik

dzerik commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Pushed the repros so you can run them directly: dzerik/sandlock branch review/pr-162-cow-repros, based on this PR's head (c712baf), test file only.

cargo test -p sandlock-core --test integration hunt_ -- --test-threads=1

Three fail here — hunt_158_fifo_commit_hang, hunt_159_opendir_whiteouted_dir_leaks_lower, hunt_160_rename_onto_existing_dir_merges — and the rest pass as the reporter-side confirmations that #161, #159's core and #160's reported case are closed.

Two notes on how they are written: the FIFO one drives run() + drop on its own OS thread and detects the hang by joining with a deadline, because commit() runs in impl Drop and a tokio timeout cannot interrupt a blocking drop; and the O_DIRECTORY one goes through the static rootfs helper, whose ls calls opendir() with no pre-stat, so it hits that open path exactly rather than being answered by the (correct) stat path. All children use only sh, coreutils and the helper — python3 does not start in every cage environment, which would otherwise mask a result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment