erofs: small cleanups to erofsmount tool - #2426
Conversation
The interface comment still said kernelDriver shells out to umount(8), which 28acdfd stopped doing when it dropped the matching Preflight requirement. Unmounts go through umount(2). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fuseDriver.Unmount tries the kernel unmount and falls back to fusermount, which is right when the mount was simply made by the other tool. It is not right when the kernel refused because the mountpoint is a symlink: fusermount has carried its own UMOUNT_NOFOLLOW since the CVE-2010-3879 fix, so it only refuses the same path again, and joining the two errors buries the reason behind fusermount's wording. kernelUnmount wraps a sentinel for that case so the caller can tell it from an ordinary failure, and Unmount returns it as-is. State-file paths already pass checkResolved, so this shrinks surface rather than fixing an exposure. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
escapeOverlayPath emits the kernel's escaping to both legs, and review asked whether fuse-overlayfs honours it. Measured against 1.17 rather than assumed, by mounting a tree whose path contains each character, escaped and verbatim, against a clean-path control: , escaped works, verbatim is split into separate options : broken both ways -- \: is not honoured as the kernel honours it \ broken both ways -- eaten, so the path never resolves So the escaping is a straight win for the comma on this leg and cannot help with the other two. It is also not the regression it looked like: a bare backslash was already eaten before the escaping went in, so both before and after, that dest fails. What is left is a bad failure mode rather than a wrong mount -- fuse-overlayfs reports "cannot resolve path .../dd" naming a path the user never wrote. Refuse ':' and '\' up front on the fall-back leg, with the reason and a pointer at --mode=kernel, which handles them correctly. The kernel overlay attempted just before the fall-back is unaffected, so only the tool that cannot cope declines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e5ad009 to
9757869
Compare
smoser
changed the title
mattmoor
left a comment
mattmoor
left a comment
There was a problem hiding this comment.
Nits only — the substance checked out: the sentinel can't strand a real mount (a race-swapped symlink means the mountpoint was already unmounted, and fusermount refuses symlinks itself), and the new check is a raw-string check on exactly the strings that reach fuse-overlayfs's -o, applied only on the leg that can't cope.
One PR-body correction: "none of this executes in CI" is overbroad. The e2e fuse leg never runs (the script goes through sudo → kernel mode), but both new unit tests do run in the go-tests job on the non-root runner — where the symlink test passes via the EPERM+lstat path, not EINVAL (see the inline comment).
| // errSymlinkedMountpoint reports a mountpoint whose final component is a | ||
| // symlink. UMOUNT_NOFOLLOW refuses those, and callers use this to tell the | ||
| // refusal apart from an ordinary unmount failure. | ||
| var errSymlinkedMountpoint = errors.New("refusing to follow a symlink") |
There was a problem hiding this comment.
This var and its comment landed between kernelUnmount's doc comment and the func — so the whole UMOUNT_NOFOLLOW rationale block above is now godoc for errSymlinkedMountpoint, and kernelUnmount is undocumented. Move the var (with its own comment) above the kernelUnmount comment block, blank line between.
There was a problem hiding this comment.
Moved in 06801ac9 — the var and its own comment now sit above the kernelUnmount block, blank line between, so the UMOUNT_NOFOLLOW rationale is godoc for the func again.
| // argument". This is for the message only -- the flag above is what | ||
| // provides the guarantee, so an lstat racing it cannot weaken anything. | ||
| if fi, lerr := os.Lstat(mp); lerr == nil && fi.Mode()&os.ModeSymlink != 0 { | ||
| return fmt.Errorf("umount %s: refusing to follow a symlink", mp) | ||
| return fmt.Errorf("umount %s: %w", mp, errSymlinkedMountpoint) |
There was a problem hiding this comment.
"This is for the message only" is no longer true: fuseDriver.Unmount now branches on the sentinel this lstat produces. Worth rewording, and one detail is worth capturing while doing so: on the non-root leg the errno here is EPERM, not EINVAL — the kernel checks may_mount() (CAP_SYS_ADMIN) before path_mounted(), so a non-root caller never sees EINVAL for a symlink. The errno-blind lstat is therefore what makes the classification work on exactly the leg commit 2 targets; a future cleanup that keys the sentinel on EINVAL would break it silently (the unit test, which runs non-root in CI, would be the only thing that notices). fuseDriver.Unmount's "Both failures are reported." doc line is also now conditionally false.
There was a problem hiding this comment.
Reworded in 06801ac9. The comment now says the lstat classifies as well as describes, and records the errno detail explicitly: as root EINVAL, but a non-root caller gets EPERM because the kernel checks may_mount() before path_mounted() — so keying the sentinel on EINVAL would stop classifying on exactly the leg the branch exists for. Unmount's "Both failures are reported" is now conditional on that branch.
| if !readOnly { | ||
| paths = append(paths, upper, work) | ||
| } | ||
| if err := checkFuseOverlayPaths(paths...); err != nil { |
There was a problem hiding this comment.
Consider running this check before the LookPath above: the path refusal is terminal (installing fuse-overlayfs won't fix a : in the dest) while "not installed" is fixable, and with that order an empty-PATH unit test can pin this wiring — raw paths, readOnly excluding upper/work — the way TestFuseDriverUnmountStopsAtASymlinkRefusal pins Unmount's. Right now TestCheckFuseOverlayPaths covers only the pure function.
There was a problem hiding this comment.
Done in 64166080 — the path check now runs first, with a comment saying why (terminal vs. fixable). TestAssembleOverlayChecksPathsBeforeLookPath uses the empty PATH to pin the wiring: a : in a lowerdir is refused rather than deferred to LookPath, a , is not (which pins that the check sees raw paths — the escaped form carries a backslash and would be refused), and read-only leaves upper/work out of the set. Mutation-checked both ways: reverting the order fails two cases, checking escaped paths fails one.
| func checkFuseOverlayPaths(paths ...string) error { | ||
| for _, p := range paths { | ||
| if i := strings.IndexAny(p, fuseOverlayUnsupported); i >= 0 { | ||
| return fmt.Errorf("fuse-overlayfs cannot handle %q in a path (%s); use --mode=kernel, or a dest without it", |
There was a problem hiding this comment.
The user on this leg is non-root by construction (resolveMode picks fuse when euid != 0, and the kernel overlay just failed above), so --mode=kernel verbatim bounces off kernelDriver.Preflight's euid check. Maybe "use --mode=kernel (as root), or a dest without it".
There was a problem hiding this comment.
Good catch — cabc8693 makes it use --mode=kernel (as root), or a dest without it.
| // So escapeOverlayPath earns its keep for the comma and cannot help with the | ||
| // other two. Refuse those rather than hand fuse-overlayfs an option string it | ||
| // will misread and then fail on with a path the user never named. | ||
| const fuseOverlayUnsupported = `:\` |
There was a problem hiding this comment.
Deliberate that this refuses on every fuse-overlayfs version with no probe, keyed to the 1.17 measurement? Fine if so — the comment records the version — just confirming the intent is "refuse until re-measured" rather than a versioned check.
There was a problem hiding this comment.
Deliberate, yes: "refuse until re-measured" rather than a versioned check. There is no probe and no version gate — 1.17 is only what the comment records as the measurement it is keyed to. If a later release fixes : or \, the table and the constant get revisited together.
The errSymlinkedMountpoint var landed between kernelUnmount's doc comment and the func, so the whole UMOUNT_NOFOLLOW rationale became godoc for the sentinel and kernelUnmount was left undocumented. Move the var, with its own comment, above the block. While there, two comments that commit ce56361 made untrue. The lstat in kernelUnmount is no longer "for the message only" -- fuseDriver. Unmount branches on the sentinel it produces -- and Unmount's "Both failures are reported" is now conditional on that branch. The lstat comment also records why it must stay errno-blind: as root the errno is EINVAL, but a non-root caller gets EPERM, because the kernel checks may_mount() before path_mounted(). So the classification would silently stop working on the fuse leg, which is the only leg that branches on it, if a later cleanup keyed it on EINVAL. Text only, no behavior change.
A path fuse-overlayfs cannot round-trip is terminal -- installing fuse-overlayfs will not make a ":" in the dest work -- while "not installed" is fixable, so report the refusal first. Ordering it this way also makes the wiring testable: with an empty PATH both the kernel overlay above and the LookPath below fail, so which error comes back pins the order, which paths are checked, and in what form. TestCheckFuseOverlayPaths covered only the pure function. The new test pins that a ":" in a lowerdir is refused rather than deferred, that a "," is not (the check sees raw paths, and the escaped form would carry a backslash), and that read-only leaves upper and work out of the set. Both directions were mutation-checked: reverting the order and checking the escaped paths each fail it.
The user reaching this refusal is non-root by construction: resolveMode picks fuse when euid != 0, and the kernel overlay just failed above. So "--mode=kernel" verbatim is advice that bounces straight off kernelDriver.Preflight's euid check.
2 participants
The three take-or-leave nits from the review of #2415, all in
pkg/erofsmount/driver_linux.go. One commit each.1. The interface comment describes an unmount that no longer happens
28acdfd7dropped theumount(8)requirement fromPreflightbut left thedriverdoc comment sayingkernelDrivershells out to it. Text only.2. Stop at a symlink refusal in fuse mode
fuseDriver.Unmounttries the kernel unmount and falls back tofusermount,which is right when the mount was simply made by the other tool and wrong when
the kernel refused because the mountpoint is a symlink.
fusermounthascarried its own
UMOUNT_NOFOLLOWsince the CVE-2010-3879 fix, so it refusesthe same path again and the joined error buries the reason:
That second line is noise once the first has decided the matter.
kernelUnmountnow wraps a sentinel for the symlink case so
Unmountcan return it as-is.State-file paths pass
checkResolvedbefore reaching a driver, so this shrinkssurface rather than fixing an exposure.
3. Refuse paths fuse-overlayfs cannot round-trip
escapeOverlayPathemits the kernel's escaping to both legs, and review askedwhether fuse-overlayfs honours it. Measured against 1.17 rather than
assumed — mounting a tree whose path contains each character, escaped and
verbatim, against a clean-path control:
DEST,:\Two things follow. The escaping is a straight win for the comma on this leg and
cannot help with the other two. And it is not the regression it looked
like: a bare backslash was already eaten before the escaping went in, so that
dest fails identically before and after.
What is left is a bad failure mode rather than a wrong mount — fuse-overlayfs
reports
cannot resolve path .../dd, naming a path the user never wrote. So:and\are refused up front on the fall-back leg, with the reason and apointer at
--mode=kernel, which handles them correctly. The kernel overlayattempted just before the fall-back is unaffected: only the tool that cannot
cope declines.
The probe script is not included — it needs a live
fuse-overlayfsand theresult is a fact about that tool, not about apko, so it is recorded as a table
in the comment on
fuseOverlayUnsupportedinstead.Verification
gofmt -lclean,golangci-lint run -nreports 0 issues,go build ./...andGOOS=darwin go build ./...both succeed, andSOURCE_DATE_EPOCH=0 go test ./...passes. The fuse-mode symlink test was checked to fail without itschange.
Both new unit tests do run in CI, in the
go-testsjob on the non-rootrunner — where the symlink test passes via the EPERM+lstat path rather than
EINVAL, since the kernel checks
may_mount()beforepath_mounted(). Whatnever runs anywhere is the e2e fuse leg:
hack/test-erofs.shgoes throughsudo, so
resolveModepicks kernel. That is one of the two items alreadyfiled against #2408.
Review follow-ups
Four commits addressing the review, one concern each:
06801ac9— moveerrSymlinkedMountpointabovekernelUnmount's doc block(it had captured the whole UMOUNT_NOFOLLOW rationale as its own godoc), and
fix the two comments commit 2 made untrue: the lstat is no longer "for the
message only", and "Both failures are reported" is now conditional. The
lstat comment records the EPERM-vs-EINVAL detail, so a later cleanup keying
it on EINVAL does not silently break the fuse leg.
64166080— runcheckFuseOverlayPathsbefore thefuse-overlayfsLookPath, since the refusal is terminal and "not installed" is fixable.With that order an empty-PATH test can pin the wiring: raw paths, and
read-only excluding upper/work. Mutation-checked both ways.
cabc8693—--mode=kernel (as root); the user on this leg is non-root byconstruction, so the bare suggestion bounces off
kernelDriver.Preflight.On the fifth comment: yes, the refusal is deliberately version-blind rather
than a versioned check — "refuse until re-measured". There is no probe, and
1.17 is what the comment records.
Refs #2408