From 84aaf53a74df008bce34314f2eb27f8863bebc1b Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 19 Aug 2026 16:56:48 -0400 Subject: [PATCH 01/18] erofs: restore mount/umount, API kept private Restores the `apko erofs mount` and `apko erofs umount` surface that 1fe041ea descoped so #2249 could land as a writer plus `ls`: the mount_linux.go orchestration, the kernel/fuse driver, the mount state file, the non-linux stubs, and their tests. Nothing here had callers outside the package, so it comes back private: the driver interface, newDriver, resolveMode, the mode type and its constants, mountState, stateSchemaVersion, statePath, writeState, loadState and removeState. Options is replaced by an exported MountOptions holding plain strings, which is all the CLI needs to say: Mount(ctx, src, dest string, opts MountOptions) error Mount returns only an error now. It used to hand back *MountState, but that type is private and the CLI discarded it anyway. Ls keeps the signature it has on main -- a plain arch string, not an Options bag -- and does not get its `--mode` flag back. That flag was documented as "accepted for symmetry with mount and ignored", which is not worth restoring. This commit deliberately restores behavior that is known to be wrong; the rest of the PR fixes it. In this state Unmount still passes whatever paths the state file names to umount as root without checking them, a partial unmount cannot be recovered by rerunning the command, and the default mount is writable and discards what was written on umount. The docs' overlayfs section is not restored: it was removed by e5778f17, the layering descope, and its recipe only works once multi-layer splitting returns. Refs #2408 Co-Authored-By: Claude Opus 5 (1M context) --- docs/erofs.md | 20 +- internal/cli/erofs.go | 75 ++++++- pkg/erofsmount/driver_linux.go | 246 +++++++++++++++++++++++ pkg/erofsmount/driver_linux_test.go | 106 ++++++++++ pkg/erofsmount/mount_linux.go | 297 ++++++++++++++++++++++++++++ pkg/erofsmount/source.go | 8 +- pkg/erofsmount/state.go | 142 +++++++++++++ pkg/erofsmount/state_test.go | 100 ++++++++++ pkg/erofsmount/stub_other.go | 44 +++++ 9 files changed, 1026 insertions(+), 12 deletions(-) create mode 100644 pkg/erofsmount/driver_linux.go create mode 100644 pkg/erofsmount/driver_linux_test.go create mode 100644 pkg/erofsmount/mount_linux.go create mode 100644 pkg/erofsmount/state.go create mode 100644 pkg/erofsmount/state_test.go create mode 100644 pkg/erofsmount/stub_other.go diff --git a/docs/erofs.md b/docs/erofs.md index 673b4cb4a..ce2d6ed7f 100644 --- a/docs/erofs.md +++ b/docs/erofs.md @@ -174,19 +174,31 @@ The merge approximates what the kernel would assemble, and diverges in two corne ## Mount the layer -A layer blob is a complete filesystem image, so mounting it takes no apko-specific tooling — either the kernel `erofs` driver (needs root) or `erofsfuse` (unprivileged): +`apko erofs mount SOURCE DEST` mounts a raw EROFS blob or an OCI image directory at `DEST`. It chooses between a kernel mount (root) and `erofsfuse` (unprivileged) based on the effective UID; use `--mode=kernel|fuse|auto` to force a choice. `--read-only` mounts the image without an upper/work overlay; for a single-layer image that means the lone layer is mounted straight at `DEST/merged` with no overlayfs in the path. `apko erofs umount DEST` tears it back down. ```sh mkdir -p /mnt/apko-erofs +apko erofs mount out/blobs/sha256/$LAYER /mnt/apko-erofs +ls /mnt/apko-erofs/ +file /mnt/apko-erofs/bin/sh +apko erofs umount /mnt/apko-erofs +``` + +If the kernel mount mode complains "unknown filesystem type 'erofs'", the kernel module is missing on your system; install it (e.g. `linux-modules-extra-$(uname -r)` on Ubuntu) or pass `--mode=fuse` to use `erofsfuse`, which does not require root and works inside CI containers that lack the kernel module. + +### Doing it manually +A layer blob is a complete filesystem image, so mounting it needs no apko-specific tooling. For reference, `apko erofs mount` is equivalent to one of: + +```sh # Kernel (root): sudo mount -t erofs -o ro out/blobs/sha256/$LAYER /mnt/apko-erofs -ls /mnt/apko-erofs/ -file /mnt/apko-erofs/bin/sh +# ...later: sudo umount /mnt/apko-erofs # FUSE (unprivileged): erofsfuse out/blobs/sha256/$LAYER /mnt/apko-erofs +# ...later: fusermount3 -u /mnt/apko-erofs # or `fusermount -u` ``` @@ -248,7 +260,7 @@ _, layer, err := bc.ImageLayoutToLayer(ctx) If you have a plain `fs.FS` and want an EROFS image, **use [go-erofs](https://github.com/erofs/go-erofs) directly** — apko doesn't expose its EROFS writer as a standalone library (and wrapping go-erofs wouldn't add meaningful value over its existing `Writer.CopyFrom(fs.FS)` API). -For inspection, apko *does* expose a focused leaf library — see `chainguard.dev/apko/pkg/erofsmount` — which provides `Stack` (layered `fs.FS` with overlay/whiteout semantics), `OpenLayers` (open an OCI EROFS image's blobs), `ReadOCILayers` (parse an OCI manifest with EROFS layers), and `Ls` (the `apko erofs ls` helper). All of it is cross-platform: go-erofs is pure Go and nothing here mounts anything. +For inspection, apko *does* expose a focused leaf library — see `chainguard.dev/apko/pkg/erofsmount` — which provides `Stack` (layered `fs.FS` with overlay/whiteout semantics), `OpenLayers` (open an OCI EROFS image's blobs), `ReadOCILayers` (parse an OCI manifest with EROFS layers), and `Mount`/`Unmount`/`Ls` (the CLI subcommand helpers, Linux-only for mount/umount; `Ls` is cross-platform). ## Current limitations diff --git a/internal/cli/erofs.go b/internal/cli/erofs.go index fa9da3bc9..358e35e20 100644 --- a/internal/cli/erofs.go +++ b/internal/cli/erofs.go @@ -22,17 +22,82 @@ import ( "chainguard.dev/apko/pkg/erofsmount" ) -// erofsCmd returns the `apko erofs` parent command, which hosts the ls -// subcommand. +// erofsCmd returns the `apko erofs` parent command, which hosts mount, umount, +// and ls subcommands. func erofsCmd() *cobra.Command { cmd := &cobra.Command{ Use: "erofs", - Short: "Inspect EROFS images produced by apko", + Short: "Mount, unmount, and inspect EROFS images produced by apko", Long: `The erofs subcommands operate on EROFS layer blobs and OCI image directories whose layers use the application/vnd.erofs mediaType (as produced -by 'apko build --format=erofs').`, +by 'apko build --format=erofs'). mount and umount are Linux-only; ls works +anywhere.`, + } + cmd.AddCommand(erofsMount(), erofsUmount(), erofsLs()) + return cmd +} + +func erofsMount() *cobra.Command { + var mode, arch string + var readOnly bool + cmd := &cobra.Command{ + Use: "mount [flags] SOURCE DEST", + Short: "Mount an EROFS blob or an EROFS OCI image at DEST", + Long: `Mount the given SOURCE at DEST. + +SOURCE may be: + - a raw EROFS blob file (mounted directly at DEST), + - an OCI image layout directory containing EROFS layers (mounted as a + multi-layer overlay rooted at DEST/merged), + - any of the above prefixed by erofs:, oci:, or oci-dir:, + - PATH:TAG to pick a manifest from a multi-tag OCI layout. + +For OCI sources, DEST gets this layout: + DEST/layers/00..NN one per EROFS layer (00 is base) + DEST/upper overlayfs upperdir (writable mounts only) + DEST/work overlayfs workdir (writable mounts only) + DEST/merged the combined view + DEST/.apko-erofs-mount.json state for 'apko erofs umount' + +With --read-only on a single-layer image, overlayfs is skipped and the +sole layer is mounted directly at DEST/merged.`, + Example: ` apko erofs mount ./out:latest /mnt/x + apko erofs mount --mode=fuse ./image.erofs /mnt/y + apko erofs mount --read-only oci-dir:./out:latest /mnt/z`, + Args: cobra.ExactArgs(2), + RunE: func(cmd *cobra.Command, args []string) error { + src, err := erofsmount.ParseSource(args[0]) + if err != nil { + return err + } + return erofsmount.Mount(cmd.Context(), src, args[1], erofsmount.MountOptions{ + Mode: mode, + Arch: arch, + ReadOnly: readOnly, + }) + }, + } + cmd.Flags().StringVar(&mode, "mode", "auto", "mount mode: kernel, fuse, or auto (auto = kernel if root else fuse)") + cmd.Flags().StringVar(&arch, "arch", "host", "architecture to select from a multi-arch OCI index (host = process arch)") + cmd.Flags().BoolVar(&readOnly, "read-only", false, "mount the image read-only (omits upperdir/workdir; single-layer images skip overlayfs entirely)") + return cmd +} + +func erofsUmount() *cobra.Command { + cmd := &cobra.Command{ + Use: "umount DEST", + Short: "Unmount an EROFS mount produced by 'apko erofs mount'", + Long: `Unmount the mount at DEST. + +If DEST contains a state file (DEST/.apko-erofs-mount.json) it is treated as +an image mount and every layer plus the overlay is torn down in reverse +order. If DEST has no state file, it is treated as a single blob mount and a +plain umount is attempted (with a fall-back to fusermount).`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return erofsmount.Unmount(cmd.Context(), args[0]) + }, } - cmd.AddCommand(erofsLs()) return cmd } diff --git a/pkg/erofsmount/driver_linux.go b/pkg/erofsmount/driver_linux.go new file mode 100644 index 000000000..d39ed81ea --- /dev/null +++ b/pkg/erofsmount/driver_linux.go @@ -0,0 +1,246 @@ +// Copyright 2026 Chainguard, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build linux + +package erofsmount + +import ( + "bytes" + "context" + "errors" + "fmt" + "os" + "os/exec" + "strings" + + "github.com/chainguard-dev/clog" +) + +// driver wraps the externally-invoked mount and umount commands used by Mount. +// Two implementations exist on Linux: kernelDriver shells out to mount(8) and +// umount(8); fuseDriver shells out to erofsfuse and fusermount. +type driver interface { + // Name returns the resolved mode (kernel or fuse), never auto. + Name() mode + // Preflight verifies that the required binaries exist and that the + // invoking process can plausibly perform mounts in this mode (e.g. + // kernelDriver requires euid 0). It must be called before any + // MountLayer/AssembleOverlay calls. + Preflight() error + // MountLayer mounts blob (a raw EROFS image) read-only at mp. The + // returned umount closure tears down that single mount. + MountLayer(ctx context.Context, blob, mp string) (func() error, error) + // AssembleOverlay layers `lowers` (in overlayfs priority order — top + // first, bottom last) on top of upper/work into merged. When readOnly is + // true, upper and work are ignored and the overlay is built as + // lowerdir-only (which overlayfs supports for read-only stacks). + AssembleOverlay(ctx context.Context, lowers []string, upper, work, merged string, readOnly bool) (func() error, error) +} + +// newDriver returns the driver that corresponds to m. m must be one of +// modeKernel or modeFuse — modeAuto must be resolved by the caller via +// resolveMode before calling newDriver. +func newDriver(m mode) (driver, error) { + switch m { + case modeKernel: + return &kernelDriver{}, nil + case modeFuse: + return &fuseDriver{}, nil + } + return nil, fmt.Errorf("unknown mount mode %q", m) +} + +// resolveMode collapses modeAuto into modeKernel (euid 0) or modeFuse. +func resolveMode(req mode) mode { + if req != modeAuto { + return req + } + if os.Geteuid() == 0 { + return modeKernel + } + return modeFuse +} + +// kernelDriver + +type kernelDriver struct{} + +func (kernelDriver) Name() mode { return modeKernel } + +func (kernelDriver) Preflight() error { + if os.Geteuid() != 0 { + return fmt.Errorf("kernel mount mode requires root (euid 0); pass --mode=fuse to use erofsfuse instead") + } + for _, bin := range []string{"mount", "umount"} { + if _, err := exec.LookPath(bin); err != nil { + return fmt.Errorf("%s not found in PATH: %w", bin, err) + } + } + return nil +} + +func (d *kernelDriver) MountLayer(ctx context.Context, blob, mp string) (func() error, error) { + args := buildKernelLayerArgs(blob, mp) + if err := runCmd(ctx, args[0], args[1:]...); err != nil { + return nil, err + } + return func() error { + uargs := buildKernelUmountArgs(mp) + return runCmd(context.Background(), uargs[0], uargs[1:]...) + }, nil +} + +func (d *kernelDriver) AssembleOverlay(ctx context.Context, lowers []string, upper, work, merged string, readOnly bool) (func() error, error) { + args := buildKernelOverlayArgs(lowers, upper, work, merged, readOnly) + if err := runCmd(ctx, args[0], args[1:]...); err != nil { + return nil, err + } + return func() error { + uargs := buildKernelUmountArgs(merged) + return runCmd(context.Background(), uargs[0], uargs[1:]...) + }, nil +} + +// fuseDriver + +type fuseDriver struct{} + +func (fuseDriver) Name() mode { return modeFuse } + +func (fuseDriver) Preflight() error { + if _, err := exec.LookPath("erofsfuse"); err != nil { + return fmt.Errorf("erofsfuse not found in PATH (install erofs-utils-fuse): %w", err) + } + if _, err := lookupFusermount(); err != nil { + return err + } + return nil +} + +func (d *fuseDriver) MountLayer(ctx context.Context, blob, mp string) (func() error, error) { + args := buildFuseLayerArgs(blob, mp) + if err := runCmd(ctx, args[0], args[1:]...); err != nil { + return nil, err + } + return func() error { + fm, err := lookupFusermount() + if err != nil { + return err + } + uargs := buildFusermountUmountArgs(fm, mp) + return runCmd(context.Background(), uargs[0], uargs[1:]...) + }, nil +} + +func (d *fuseDriver) AssembleOverlay(ctx context.Context, lowers []string, upper, work, merged string, readOnly bool) (func() error, error) { + // First try the kernel overlay driver on top of the FUSE lowerdirs. Modern + // kernels (~5.11+) allow this in user namespaces. If that fails, fall back + // to fuse-overlayfs. + kArgs := buildKernelOverlayArgs(lowers, upper, work, merged, readOnly) + if err := runCmd(ctx, kArgs[0], kArgs[1:]...); err == nil { + return func() error { + uargs := buildKernelUmountArgs(merged) + return runCmd(context.Background(), uargs[0], uargs[1:]...) + }, nil + } + + if _, err := exec.LookPath("fuse-overlayfs"); err != nil { + return nil, fmt.Errorf("kernel overlay failed and fuse-overlayfs is not installed: %w", err) + } + fArgs := buildFuseOverlayArgs(lowers, upper, work, merged, readOnly) + if err := runCmd(ctx, fArgs[0], fArgs[1:]...); err != nil { + return nil, err + } + return func() error { + fm, err := lookupFusermount() + if err != nil { + return err + } + uargs := buildFusermountUmountArgs(fm, merged) + return runCmd(context.Background(), uargs[0], uargs[1:]...) + }, nil +} + +// Command builders. Pure functions so they can be tested without exec. + +func buildKernelLayerArgs(blob, mp string) []string { + // "-o loop" is unnecessary on modern util-linux: when the source is a + // regular file, mount(8) auto-detects and allocates a loop device with + // O_AUTOCLEAR so it's freed on umount. Asking for "-o loop" explicitly + // risks leaking the loop device when the kernel/util-linux don't agree + // on autoclear semantics. EROFS itself is read-only, but pass "-o ro" + // anyway to document intent. + return []string{"mount", "-t", "erofs", "-o", "ro", blob, mp} +} + +func buildKernelUmountArgs(mp string) []string { + return []string{"umount", mp} +} + +func buildFuseLayerArgs(blob, mp string) []string { + return []string{"erofsfuse", blob, mp} +} + +func buildFusermountUmountArgs(fusermountBin, mp string) []string { + return []string{fusermountBin, "-u", mp} +} + +func buildKernelOverlayArgs(lowers []string, upper, work, merged string, readOnly bool) []string { + opts := "lowerdir=" + strings.Join(lowers, ":") + if !readOnly { + opts += ",upperdir=" + upper + ",workdir=" + work + } else { + opts += ",ro" + } + return []string{"mount", "-t", "overlay", "-o", opts, "overlay", merged} +} + +func buildFuseOverlayArgs(lowers []string, upper, work, merged string, readOnly bool) []string { + opts := "lowerdir=" + strings.Join(lowers, ":") + if !readOnly { + opts += ",upperdir=" + upper + ",workdir=" + work + } + return []string{"fuse-overlayfs", "-o", opts, merged} +} + +// lookupFusermount returns the path to whichever of `fusermount3` or +// `fusermount` is available, preferring fusermount3 since it matches modern +// libfuse builds. +func lookupFusermount() (string, error) { + for _, name := range []string{"fusermount3", "fusermount"} { + if path, err := exec.LookPath(name); err == nil { + return path, nil + } + } + return "", errors.New("neither fusermount3 nor fusermount found in PATH") +} + +// runCmd runs name+args, captures stderr, and wraps any error with the +// captured stderr so users see what mount(8) actually said. +func runCmd(ctx context.Context, name string, args ...string) error { + log := clog.FromContext(ctx) + cmd := exec.CommandContext(ctx, name, args...) + var stderr bytes.Buffer + cmd.Stderr = &stderr + log.Debugf("exec: %s %s", name, strings.Join(args, " ")) + if err := cmd.Run(); err != nil { + stderrTrim := strings.TrimSpace(stderr.String()) + if stderrTrim != "" { + return fmt.Errorf("%s %s: %w: %s", name, strings.Join(args, " "), err, stderrTrim) + } + return fmt.Errorf("%s %s: %w", name, strings.Join(args, " "), err) + } + return nil +} diff --git a/pkg/erofsmount/driver_linux_test.go b/pkg/erofsmount/driver_linux_test.go new file mode 100644 index 000000000..feaecbdb0 --- /dev/null +++ b/pkg/erofsmount/driver_linux_test.go @@ -0,0 +1,106 @@ +// Copyright 2026 Chainguard, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build linux + +package erofsmount + +import ( + "reflect" + "strings" + "testing" +) + +func TestBuildKernelLayerArgs(t *testing.T) { + got := buildKernelLayerArgs("/blobs/abc", "/mnt/x") + want := []string{"mount", "-t", "erofs", "-o", "ro", "/blobs/abc", "/mnt/x"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v, want %v", got, want) + } +} + +func TestBuildFuseLayerArgs(t *testing.T) { + got := buildFuseLayerArgs("/blobs/abc", "/mnt/x") + want := []string{"erofsfuse", "/blobs/abc", "/mnt/x"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v, want %v", got, want) + } +} + +func TestBuildKernelOverlayArgs_Writable(t *testing.T) { + got := buildKernelOverlayArgs( + []string{"/mnt/x/layers/02", "/mnt/x/layers/01", "/mnt/x/layers/00"}, + "/mnt/x/upper", "/mnt/x/work", "/mnt/x/merged", + false, + ) + want := []string{ + "mount", "-t", "overlay", "-o", + "lowerdir=/mnt/x/layers/02:/mnt/x/layers/01:/mnt/x/layers/00,upperdir=/mnt/x/upper,workdir=/mnt/x/work", + "overlay", "/mnt/x/merged", + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v\nwant %v", got, want) + } +} + +func TestBuildKernelOverlayArgs_ReadOnly(t *testing.T) { + got := buildKernelOverlayArgs( + []string{"/a", "/b"}, + "/ignored-upper", "/ignored-work", "/merged", + true, + ) + // Read-only must omit upperdir/workdir and append ,ro. + opts := got[4] + if strings.Contains(opts, "upperdir") || strings.Contains(opts, "workdir") { + t.Errorf("read-only overlay should not reference upperdir/workdir: %s", opts) + } + if !strings.HasSuffix(opts, ",ro") { + t.Errorf("read-only overlay opts should end with ,ro: %s", opts) + } + if !strings.HasPrefix(opts, "lowerdir=/a:/b") { + t.Errorf("lowerdir order wrong: %s", opts) + } +} + +func TestBuildFuseOverlayArgs(t *testing.T) { + got := buildFuseOverlayArgs( + []string{"/a", "/b"}, + "/u", "/w", "/m", + false, + ) + want := []string{ + "fuse-overlayfs", "-o", + "lowerdir=/a:/b,upperdir=/u,workdir=/w", + "/m", + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v\nwant %v", got, want) + } +} + +func TestBuildKernelUmountArgs(t *testing.T) { + got := buildKernelUmountArgs("/mnt/x") + want := []string{"umount", "/mnt/x"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v, want %v", got, want) + } +} + +func TestBuildFusermountUmountArgs(t *testing.T) { + got := buildFusermountUmountArgs("/usr/bin/fusermount3", "/mnt/x") + want := []string{"/usr/bin/fusermount3", "-u", "/mnt/x"} + if !reflect.DeepEqual(got, want) { + t.Fatalf("got %v, want %v", got, want) + } +} diff --git a/pkg/erofsmount/mount_linux.go b/pkg/erofsmount/mount_linux.go new file mode 100644 index 000000000..ad90a6b9a --- /dev/null +++ b/pkg/erofsmount/mount_linux.go @@ -0,0 +1,297 @@ +// Copyright 2026 Chainguard, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build linux + +package erofsmount + +import ( + "context" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "slices" + "time" + + "github.com/chainguard-dev/clog" +) + +// Mount mounts src at dest. For KindBlob, dest is the single mountpoint. For +// KindOCIDir, dest is a directory that receives the standard layout: +// +// /layers/00..NN per-layer EROFS mounts (00 is base) +// /upper overlayfs upperdir (writable mounts only) +// /work overlayfs workdir (writable mounts only) +// /merged overlayfs merged view +// /.apko-erofs-mount.json state record for Unmount +// +// The mount is recorded in a state file that Unmount reads back; that record +// is internal, so Mount reports only an error. On any error after a partial +// mount, all partially-completed mounts are torn down before returning. +func Mount(ctx context.Context, src Source, dest string, opts MountOptions) error { + log := clog.FromContext(ctx) + + absDest, err := filepath.Abs(dest) + if err != nil { + return fmt.Errorf("resolve dest: %w", err) + } + dest = filepath.Clean(absDest) + + if opts.Mode == "" { + opts.Mode = string(modeAuto) + } + drv, err := newDriver(resolveMode(mode(opts.Mode))) + if err != nil { + return err + } + if err := drv.Preflight(); err != nil { + return err + } + + switch src.Kind { + case KindBlob: + return mountBlob(ctx, drv, src, dest, log) + case KindOCIDir: + return mountImage(ctx, drv, src, dest, opts, log) + } + return fmt.Errorf("unsupported source kind: %v", src.Kind) +} + +func mountBlob(ctx context.Context, drv driver, src Source, dest string, log *clog.Logger) error { + if err := ensureDir(dest); err != nil { + return err + } + if _, err := drv.MountLayer(ctx, src.Path, dest); err != nil { + return fmt.Errorf("mount %s at %s: %w", src.Path, dest, err) + } + log.Infof("mounted %s at %s (%s)", src.Path, dest, drv.Name()) + // No state file for raw blobs: there is no enclosing directory to put one + // in. See Unmount for the matching teardown logic. + return nil +} + +func mountImage(ctx context.Context, drv driver, src Source, dest string, opts MountOptions, log *clog.Logger) (retErr error) { + layers, err := ReadOCILayers(src.Path, src.Tag, opts.Arch) + if err != nil { + return err + } + + // Refuse to clobber an existing mount. + if _, err := os.Stat(statePath(dest)); err == nil { + return fmt.Errorf("dest %s already has a mount state file (%s); umount first", dest, statePath(dest)) + } else if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("stat state file: %w", err) + } + + var cleanups []func() error + defer func() { + if retErr == nil { + return + } + for _, c := range slices.Backward(cleanups) { + if err := c(); err != nil { + log.Warnf("cleanup on error: %v", err) + } + } + }() + + // Single-layer read-only short-circuit: overlay buys nothing when there's + // one lower and no upper, so mount the layer straight at DEST/merged. + // Multi-layer and writable mounts still compose through overlayfs. + if opts.ReadOnly && len(layers) == 1 { + merged := filepath.Join(dest, "merged") + if err := ensureDir(merged); err != nil { + return err + } + umount, err := drv.MountLayer(ctx, layers[0].BlobPath, merged) + if err != nil { + return fmt.Errorf("mount layer 0 (%s) at %s: %w", layers[0].Digest, merged, err) + } + cleanups = append(cleanups, umount) + log.Infof("mounted single layer (%s) read-only at %s", layers[0].Digest, merged) + + state := &mountState{ + SchemaVersion: stateSchemaVersion, + Mode: drv.Name(), + Source: src.Raw, + Dest: dest, + Created: time.Now().UTC(), + Mounts: []string{merged}, + } + if err := writeState(dest, state); err != nil { + return fmt.Errorf("write state: %w", err) + } + return nil + } + + for _, sub := range []string{"layers", "upper", "work", "merged"} { + if err := ensureDir(filepath.Join(dest, sub)); err != nil { + return err + } + } + + layerMps := make([]string, 0, len(layers)) + mountsLIFO := make([]string, 0, len(layers)+1) + for i, layer := range layers { + mp := filepath.Join(dest, "layers", fmt.Sprintf("%02d", i)) + if err := ensureDir(mp); err != nil { + return err + } + umount, err := drv.MountLayer(ctx, layer.BlobPath, mp) + if err != nil { + return fmt.Errorf("mount layer %d (%s) at %s: %w", i, layer.Digest, mp, err) + } + cleanups = append(cleanups, umount) + layerMps = append(layerMps, mp) + mountsLIFO = append([]string{mp}, mountsLIFO...) + log.Infof("mounted layer %d (%s) at %s", i, layer.Digest, mp) + } + + // overlayfs lowerdir is highest-priority first; OCI is bottom-up so we + // reverse. + lowers := make([]string, len(layerMps)) + for i := range layerMps { + lowers[i] = layerMps[len(layerMps)-1-i] + } + + upper := filepath.Join(dest, "upper") + work := filepath.Join(dest, "work") + merged := filepath.Join(dest, "merged") + umount, err := drv.AssembleOverlay(ctx, lowers, upper, work, merged, opts.ReadOnly) + if err != nil { + return fmt.Errorf("overlay merge into %s: %w", merged, err) + } + cleanups = append(cleanups, umount) + mountsLIFO = append([]string{merged}, mountsLIFO...) + log.Infof("merged %d layer(s) at %s", len(layers), merged) + + state := &mountState{ + SchemaVersion: stateSchemaVersion, + Mode: drv.Name(), + Source: src.Raw, + Dest: dest, + Created: time.Now().UTC(), + Mounts: mountsLIFO, + } + if err := writeState(dest, state); err != nil { + return fmt.Errorf("write state: %w", err) + } + return nil +} + +// Unmount tears down a mount produced by Mount. For an image mount it reads +// the state file at /.apko-erofs-mount.json and unmounts in LIFO order; +// if the state file is absent it falls back to treating dest as a single +// (blob) mountpoint and runs umount/fusermount. +func Unmount(ctx context.Context, dest string) error { + log := clog.FromContext(ctx) + absDest, err := filepath.Abs(dest) + if err != nil { + return fmt.Errorf("resolve dest: %w", err) + } + dest = filepath.Clean(absDest) + + st, err := loadState(dest) + if err == nil { + return unmountImage(ctx, dest, st, log) + } + if !errors.Is(err, fs.ErrNotExist) { + return err + } + return unmountBlob(ctx, dest, log) +} + +func unmountImage(ctx context.Context, dest string, st *mountState, log *clog.Logger) error { + drv, err := newDriver(st.Mode) + if err != nil { + return err + } + // st.Mounts is overlay-first then per-layer mounts in LIFO order. If + // any umount fails, stop: layer mounts that come after a still-pinned + // overlay would only return EBUSY noise, and continuing past an error + // would also leave the state file out of sync with reality. The user + // can rerun `apko erofs umount` after addressing whatever is keeping + // the mount busy. + for _, mp := range st.Mounts { + if err := unmountOne(ctx, drv, mp); err != nil { + return fmt.Errorf("umount %s: %w (remaining mounts left intact; rerun once they are no longer busy)", mp, err) + } + log.Infof("unmounted %s", mp) + } + for _, sub := range []string{"merged", "upper", "work", "layers"} { + if err := os.RemoveAll(filepath.Join(dest, sub)); err != nil { + log.Warnf("remove %s: %v", filepath.Join(dest, sub), err) + } + } + if err := removeState(dest); err != nil { + return fmt.Errorf("remove state file: %w", err) + } + return nil +} + +// unmountBlob tears down a single mountpoint produced by mountBlob. Since +// blobs do not have a state file we have to guess which umount tool applies; +// we try kernel umount first (which works for both kernel-erofs and any +// kernel-overlay-over-fuse cases by transitively triggering fuse teardown +// where appropriate) and fall back to fusermount. +func unmountBlob(ctx context.Context, dest string, log *clog.Logger) error { + if err := runCmd(ctx, "umount", dest); err == nil { + log.Infof("unmounted %s", dest) + return nil + } + fm, err := lookupFusermount() + if err != nil { + return fmt.Errorf("umount %s: kernel umount failed and no fusermount available", dest) + } + if err := runCmd(ctx, fm, "-u", dest); err != nil { + return fmt.Errorf("umount %s: %w", dest, err) + } + log.Infof("unmounted %s", dest) + return nil +} + +// unmountOne unmounts mp using the appropriate tool for the recorded driver. +// We don't try to be clever about kernel-vs-fuse here beyond what the state +// file tells us; if the user mounted with kernel they need kernel umount. +func unmountOne(ctx context.Context, drv driver, mp string) error { + switch drv.Name() { + case modeKernel: + return runCmd(ctx, "umount", mp) + case modeFuse: + // For fuse mounts: the merged view may itself be a kernel overlay + // (when overlayfs over FUSE worked) or a fuse-overlayfs mount. + // `umount` handles both kernel-side overlays; `fusermount -u` + // handles fuse-overlayfs and the per-layer erofsfuse mounts. Try + // kernel umount first (cheap, no-op if not applicable), then + // fusermount. + if err := runCmd(ctx, "umount", mp); err == nil { + return nil + } + fm, err := lookupFusermount() + if err != nil { + return err + } + return runCmd(ctx, fm, "-u", mp) + } + return fmt.Errorf("unknown mode %q", drv.Name()) +} + +func ensureDir(path string) error { + if err := os.MkdirAll(path, 0o755); err != nil { + return fmt.Errorf("mkdir %s: %w", path, err) + } + return nil +} diff --git a/pkg/erofsmount/source.go b/pkg/erofsmount/source.go index e6491ea9b..cb051958c 100644 --- a/pkg/erofsmount/source.go +++ b/pkg/erofsmount/source.go @@ -13,9 +13,11 @@ // limitations under the License. // Package erofsmount provides the building blocks for the `apko erofs` -// subcommands. It exposes a small library: parse a source spec, read an OCI -// layout's EROFS layers, and present them as a single merged fs.FS. Everything -// here is pure Go and cross-platform; nothing mounts anything. +// subcommands (mount, umount, ls). It exposes a small library: parse a source +// spec, read an OCI layout's EROFS layers, present them as a single merged +// fs.FS, and drive kernel or FUSE mounts. The reading side is pure Go and +// cross-platform; Mount and Unmount are implemented for Linux only and return +// an error elsewhere. package erofsmount import ( diff --git a/pkg/erofsmount/state.go b/pkg/erofsmount/state.go new file mode 100644 index 000000000..cdd1025e0 --- /dev/null +++ b/pkg/erofsmount/state.go @@ -0,0 +1,142 @@ +// Copyright 2026 Chainguard, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package erofsmount + +import ( + "encoding/json" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "time" +) + +// mode selects how mounts are performed. modeAuto is resolved to modeKernel or +// modeFuse before being recorded in mountState. +type mode string + +const ( + modeAuto mode = "auto" + modeKernel mode = "kernel" + modeFuse mode = "fuse" +) + +// MountOptions is the option bag accepted by Mount. It is the only exported +// part of the mount plane's configuration: mode is an internal type, so the +// Mode field is a plain string converted on the way in. +type MountOptions struct { + // Mode selects "kernel", "fuse", or "auto". Zero value is treated as + // "auto" (kernel if euid 0, else fuse). + Mode string + // Arch picks a manifest from a multi-arch OCI index. "" or "host" + // means runtime.GOARCH. + Arch string + // ReadOnly, when true, skips upper/work overlay dirs and produces a + // pure read-only overlay during Mount. + ReadOnly bool +} + +// stateSchemaVersion is the current mountState JSON schema version. +const stateSchemaVersion = 1 + +// stateFileName is written inside for image mounts (multi-layer overlay +// or single-layer wrapped in an OCI layout). It is *not* written for raw blob +// mounts: there is no enclosing directory for them. +const stateFileName = ".apko-erofs-mount.json" + +// mountState describes a completed mount produced by Mount. The file +// authoritatively records what was mounted so that Unmount can tear it down +// without re-deriving the layout from the source. +type mountState struct { + SchemaVersion int `json:"schemaVersion"` + Mode mode `json:"mode"` // resolved mode (kernel|fuse), never "auto" + Source string `json:"source"` // the original `spec` argument + Dest string `json:"dest"` // absolute path of the mount target + Created time.Time `json:"created"` // wall-clock timestamp at mount completion + // Mounts lists every mountpoint produced by Mount in unmount order + // (LIFO): the first element is unmounted first. For an image mount this + // is [/merged, /layers/NN, ..., /layers/00]. + Mounts []string `json:"mounts"` +} + +// statePath returns the location of the state file inside dest. +func statePath(dest string) string { + return filepath.Join(dest, stateFileName) +} + +// writeState writes s atomically to statePath(dest). The file is written via +// CreateTemp+Rename in the same directory so a partial write can never be +// observed. +func writeState(dest string, s *mountState) error { + path := statePath(dest) + tmp, err := os.CreateTemp(filepath.Dir(path), ".apko-erofs-mount-*.json") + if err != nil { + return fmt.Errorf("create state tmpfile: %w", err) + } + tmpName := tmp.Name() + defer func() { + // Best-effort cleanup if Rename never happened. + _ = os.Remove(tmpName) + }() + enc := json.NewEncoder(tmp) + enc.SetIndent("", " ") + if err := enc.Encode(s); err != nil { + _ = tmp.Close() + return fmt.Errorf("encode state: %w", err) + } + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + return fmt.Errorf("sync state: %w", err) + } + if err := tmp.Close(); err != nil { + return fmt.Errorf("close state: %w", err) + } + if err := os.Rename(tmpName, path); err != nil { + return fmt.Errorf("rename state into place: %w", err) + } + return nil +} + +// loadState reads statePath(dest). If the file does not exist, the returned +// error wraps fs.ErrNotExist so callers can use errors.Is. +func loadState(dest string) (*mountState, error) { + path := statePath(dest) + data, err := os.ReadFile(path) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil, fmt.Errorf("no mount state at %s: %w", path, err) + } + return nil, fmt.Errorf("read state %s: %w", path, err) + } + var s mountState + if err := json.Unmarshal(data, &s); err != nil { + return nil, fmt.Errorf("parse state %s: %w", path, err) + } + if s.SchemaVersion != stateSchemaVersion { + return nil, fmt.Errorf("state %s: unsupported schemaVersion %d (want %d)", path, s.SchemaVersion, stateSchemaVersion) + } + return &s, nil +} + +// removeState deletes statePath(dest). It is a no-op if the file is already +// absent. +func removeState(dest string) error { + err := os.Remove(statePath(dest)) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return err + } + return nil +} diff --git a/pkg/erofsmount/state_test.go b/pkg/erofsmount/state_test.go new file mode 100644 index 000000000..0bfca48e6 --- /dev/null +++ b/pkg/erofsmount/state_test.go @@ -0,0 +1,100 @@ +// Copyright 2026 Chainguard, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package erofsmount + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + "reflect" + "testing" + "time" +) + +func TestStateRoundTrip(t *testing.T) { + dest := t.TempDir() + in := &mountState{ + SchemaVersion: stateSchemaVersion, + Mode: modeKernel, + Source: "oci-dir:./out:latest", + Dest: dest, + Created: time.Date(2026, 5, 27, 12, 0, 0, 0, time.UTC), + Mounts: []string{ + filepath.Join(dest, "merged"), + filepath.Join(dest, "layers", "02"), + filepath.Join(dest, "layers", "01"), + filepath.Join(dest, "layers", "00"), + }, + } + if err := writeState(dest, in); err != nil { + t.Fatalf("writeState: %v", err) + } + if _, err := os.Stat(statePath(dest)); err != nil { + t.Fatalf("state file missing: %v", err) + } + + out, err := loadState(dest) + if err != nil { + t.Fatalf("loadState: %v", err) + } + if !reflect.DeepEqual(in, out) { + t.Fatalf("roundtrip mismatch:\n in=%+v\n out=%+v", in, out) + } + + // No leftover tempfile from the atomic write. + entries, err := os.ReadDir(dest) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + name := e.Name() + if len(name) > len(".apko-erofs-mount-") && name[:len(".apko-erofs-mount-")] == ".apko-erofs-mount-" { + t.Errorf("stray tempfile left behind: %s", name) + } + } + + if err := removeState(dest); err != nil { + t.Fatalf("removeState: %v", err) + } + if _, err := os.Stat(statePath(dest)); !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("state still present after remove: err=%v", err) + } + // Idempotent remove. + if err := removeState(dest); err != nil { + t.Fatalf("removeState (idempotent): %v", err) + } +} + +func TestLoadStateMissing(t *testing.T) { + dest := t.TempDir() + _, err := loadState(dest) + if err == nil { + t.Fatal("expected error") + } + if !errors.Is(err, fs.ErrNotExist) { + t.Fatalf("error %v should wrap fs.ErrNotExist", err) + } +} + +func TestLoadStateWrongSchema(t *testing.T) { + dest := t.TempDir() + if err := os.WriteFile(statePath(dest), []byte(`{"schemaVersion":99}`), 0o600); err != nil { + t.Fatal(err) + } + if _, err := loadState(dest); err == nil { + t.Fatal("expected schema version error") + } +} diff --git a/pkg/erofsmount/stub_other.go b/pkg/erofsmount/stub_other.go new file mode 100644 index 000000000..c61536840 --- /dev/null +++ b/pkg/erofsmount/stub_other.go @@ -0,0 +1,44 @@ +// Copyright 2026 Chainguard, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build !linux + +package erofsmount + +import ( + "context" + "fmt" + "runtime" +) + +// The driver interface, newDriver and resolveMode are intentionally absent on +// non-Linux: the EROFS kernel module, erofsfuse, overlayfs, and fuse-overlayfs +// are Linux concepts. Mount and Unmount return a clear error so the CLI doesn't +// have to gate at every call site. +// +// Ls and OpenLayers are cross-platform (go-erofs is pure Go). + +func unsupportedOS() error { + return fmt.Errorf("apko erofs mount/umount are only supported on Linux (running on %s)", runtime.GOOS) +} + +// Mount is a no-op stub on non-Linux that returns an error. +func Mount(_ context.Context, _ Source, _ string, _ MountOptions) error { + return unsupportedOS() +} + +// Unmount is a no-op stub on non-Linux that returns an error. +func Unmount(_ context.Context, _ string) error { + return unsupportedOS() +} From 7c4c73550a29c312967ebbe0e17c744714bd7d12 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 19 Aug 2026 17:06:02 -0400 Subject: [PATCH 02/18] erofsmount: put a test seam under the orchestration Mount and Unmount built their driver internally, so the ~300 lines of orchestration around it -- cleanup LIFO, state file lifecycle, unmount policy -- could not be driven with a fake. driver_linux_test.go covered only the argv builders, and nothing anywhere executed the orchestration at all. Add a driverFactory type and unexported mountWith/unmountWith that take it; the exported Mount and Unmount now just pass newDriver. Tests pass a factory returning a fake that records calls. The kernel-vs-fuse umount choice moves into the driver as a new Unmount(ctx, mp) method, replacing unmountOne's switch on drv.Name(). That also puts unmountBlob behind the seam: it now calls the fuse driver's Unmount, which is already the kernel-umount-then-fusermount chain unmountBlob was open-coding, so a blob teardown no longer shells out from the orchestration layer. Behavior is unchanged. Six tests, all through a fake: mount order and recorded state for a three-layer image, lowerdir reversal, LIFO teardown with no state file left behind when overlay assembly fails, refusal to clobber an existing state file, the single-layer read-only short circuit skipping overlay entirely, unmount following the recorded order and cleaning up, and a stateless dest being treated as a blob mountpoint. This lands first because the fixes that follow -- state file containment, partial-unmount recovery, a read-only default -- are only testable through it. Refs #2408 Co-Authored-By: Claude Opus 5 (1M context) --- pkg/erofsmount/driver_linux.go | 32 ++++ pkg/erofsmount/mount_linux.go | 71 +++----- pkg/erofsmount/mount_linux_test.go | 274 +++++++++++++++++++++++++++++ 3 files changed, 332 insertions(+), 45 deletions(-) create mode 100644 pkg/erofsmount/mount_linux_test.go diff --git a/pkg/erofsmount/driver_linux.go b/pkg/erofsmount/driver_linux.go index d39ed81ea..905507b0f 100644 --- a/pkg/erofsmount/driver_linux.go +++ b/pkg/erofsmount/driver_linux.go @@ -47,8 +47,18 @@ type driver interface { // true, upper and work are ignored and the overlay is built as // lowerdir-only (which overlayfs supports for read-only stacks). AssembleOverlay(ctx context.Context, lowers []string, upper, work, merged string, readOnly bool) (func() error, error) + // Unmount tears down a single mountpoint with the tool this driver + // mounts with. Unlike the closures returned by MountLayer and + // AssembleOverlay it needs no prior call in this process, so it is what + // Unmount uses when working from a state file. + Unmount(ctx context.Context, mp string) error } +// driverFactory builds the driver for a mode. Mount and Unmount pass newDriver; +// tests pass a factory returning a fake, which is the only way to exercise the +// mount and unmount orchestration without root and real filesystems. +type driverFactory func(m mode) (driver, error) + // newDriver returns the driver that corresponds to m. m must be one of // modeKernel or modeFuse — modeAuto must be resolved by the caller via // resolveMode before calling newDriver. @@ -102,6 +112,11 @@ func (d *kernelDriver) MountLayer(ctx context.Context, blob, mp string) (func() }, nil } +func (d *kernelDriver) Unmount(ctx context.Context, mp string) error { + uargs := buildKernelUmountArgs(mp) + return runCmd(ctx, uargs[0], uargs[1:]...) +} + func (d *kernelDriver) AssembleOverlay(ctx context.Context, lowers []string, upper, work, merged string, readOnly bool) (func() error, error) { args := buildKernelOverlayArgs(lowers, upper, work, merged, readOnly) if err := runCmd(ctx, args[0], args[1:]...); err != nil { @@ -144,6 +159,23 @@ func (d *fuseDriver) MountLayer(ctx context.Context, blob, mp string) (func() er }, nil } +// Unmount tries kernel umount before fusermount. A merged view mounted in fuse +// mode may itself be a kernel overlay (when overlayfs over FUSE worked) or a +// fuse-overlayfs mount, and per-layer mounts are erofsfuse; umount handles the +// first, fusermount -u the other two. +func (d *fuseDriver) Unmount(ctx context.Context, mp string) error { + uargs := buildKernelUmountArgs(mp) + if err := runCmd(ctx, uargs[0], uargs[1:]...); err == nil { + return nil + } + fm, err := lookupFusermount() + if err != nil { + return err + } + fargs := buildFusermountUmountArgs(fm, mp) + return runCmd(ctx, fargs[0], fargs[1:]...) +} + func (d *fuseDriver) AssembleOverlay(ctx context.Context, lowers []string, upper, work, merged string, readOnly bool) (func() error, error) { // First try the kernel overlay driver on top of the FUSE lowerdirs. Modern // kernels (~5.11+) allow this in user namespaces. If that fails, fall back diff --git a/pkg/erofsmount/mount_linux.go b/pkg/erofsmount/mount_linux.go index ad90a6b9a..6230de924 100644 --- a/pkg/erofsmount/mount_linux.go +++ b/pkg/erofsmount/mount_linux.go @@ -42,6 +42,12 @@ import ( // is internal, so Mount reports only an error. On any error after a partial // mount, all partially-completed mounts are torn down before returning. func Mount(ctx context.Context, src Source, dest string, opts MountOptions) error { + return mountWith(ctx, newDriver, src, dest, opts) +} + +// mountWith is Mount with the driver constructor injected, so tests can drive +// the orchestration -- cleanup ordering, state file lifecycle -- with a fake. +func mountWith(ctx context.Context, newDrv driverFactory, src Source, dest string, opts MountOptions) error { log := clog.FromContext(ctx) absDest, err := filepath.Abs(dest) @@ -53,7 +59,7 @@ func Mount(ctx context.Context, src Source, dest string, opts MountOptions) erro if opts.Mode == "" { opts.Mode = string(modeAuto) } - drv, err := newDriver(resolveMode(mode(opts.Mode))) + drv, err := newDrv(resolveMode(mode(opts.Mode))) if err != nil { return err } @@ -197,6 +203,11 @@ func mountImage(ctx context.Context, drv driver, src Source, dest string, opts M // if the state file is absent it falls back to treating dest as a single // (blob) mountpoint and runs umount/fusermount. func Unmount(ctx context.Context, dest string) error { + return unmountWith(ctx, newDriver, dest) +} + +// unmountWith is Unmount with the driver constructor injected. See mountWith. +func unmountWith(ctx context.Context, newDrv driverFactory, dest string) error { log := clog.FromContext(ctx) absDest, err := filepath.Abs(dest) if err != nil { @@ -206,16 +217,16 @@ func Unmount(ctx context.Context, dest string) error { st, err := loadState(dest) if err == nil { - return unmountImage(ctx, dest, st, log) + return unmountImage(ctx, newDrv, dest, st, log) } if !errors.Is(err, fs.ErrNotExist) { return err } - return unmountBlob(ctx, dest, log) + return unmountBlob(ctx, newDrv, dest, log) } -func unmountImage(ctx context.Context, dest string, st *mountState, log *clog.Logger) error { - drv, err := newDriver(st.Mode) +func unmountImage(ctx context.Context, newDrv driverFactory, dest string, st *mountState, log *clog.Logger) error { + drv, err := newDrv(st.Mode) if err != nil { return err } @@ -226,7 +237,7 @@ func unmountImage(ctx context.Context, dest string, st *mountState, log *clog.Lo // can rerun `apko erofs umount` after addressing whatever is keeping // the mount busy. for _, mp := range st.Mounts { - if err := unmountOne(ctx, drv, mp); err != nil { + if err := drv.Unmount(ctx, mp); err != nil { return fmt.Errorf("umount %s: %w (remaining mounts left intact; rerun once they are no longer busy)", mp, err) } log.Infof("unmounted %s", mp) @@ -242,53 +253,23 @@ func unmountImage(ctx context.Context, dest string, st *mountState, log *clog.Lo return nil } -// unmountBlob tears down a single mountpoint produced by mountBlob. Since -// blobs do not have a state file we have to guess which umount tool applies; -// we try kernel umount first (which works for both kernel-erofs and any -// kernel-overlay-over-fuse cases by transitively triggering fuse teardown -// where appropriate) and fall back to fusermount. -func unmountBlob(ctx context.Context, dest string, log *clog.Logger) error { - if err := runCmd(ctx, "umount", dest); err == nil { - log.Infof("unmounted %s", dest) - return nil - } - fm, err := lookupFusermount() +// unmountBlob tears down a single mountpoint produced by mountBlob. Blobs have +// no state file, so the mode they were mounted with is unknown. The fuse +// driver's Unmount is already the kernel-umount-then-fusermount chain that +// covers both cases, so it is what we use here regardless of how the mount was +// made. +func unmountBlob(ctx context.Context, newDrv driverFactory, dest string, log *clog.Logger) error { + drv, err := newDrv(modeFuse) if err != nil { - return fmt.Errorf("umount %s: kernel umount failed and no fusermount available", dest) + return err } - if err := runCmd(ctx, fm, "-u", dest); err != nil { + if err := drv.Unmount(ctx, dest); err != nil { return fmt.Errorf("umount %s: %w", dest, err) } log.Infof("unmounted %s", dest) return nil } -// unmountOne unmounts mp using the appropriate tool for the recorded driver. -// We don't try to be clever about kernel-vs-fuse here beyond what the state -// file tells us; if the user mounted with kernel they need kernel umount. -func unmountOne(ctx context.Context, drv driver, mp string) error { - switch drv.Name() { - case modeKernel: - return runCmd(ctx, "umount", mp) - case modeFuse: - // For fuse mounts: the merged view may itself be a kernel overlay - // (when overlayfs over FUSE worked) or a fuse-overlayfs mount. - // `umount` handles both kernel-side overlays; `fusermount -u` - // handles fuse-overlayfs and the per-layer erofsfuse mounts. Try - // kernel umount first (cheap, no-op if not applicable), then - // fusermount. - if err := runCmd(ctx, "umount", mp); err == nil { - return nil - } - fm, err := lookupFusermount() - if err != nil { - return err - } - return runCmd(ctx, fm, "-u", mp) - } - return fmt.Errorf("unknown mode %q", drv.Name()) -} - func ensureDir(path string) error { if err := os.MkdirAll(path, 0o755); err != nil { return fmt.Errorf("mkdir %s: %w", path, err) diff --git a/pkg/erofsmount/mount_linux_test.go b/pkg/erofsmount/mount_linux_test.go new file mode 100644 index 000000000..8c2bc45c2 --- /dev/null +++ b/pkg/erofsmount/mount_linux_test.go @@ -0,0 +1,274 @@ +// Copyright 2026 Chainguard, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build linux + +package erofsmount + +import ( + "context" + "errors" + "os" + "path/filepath" + "slices" + "testing" + + "chainguard.dev/apko/pkg/build/types" +) + +// fakeDriver records what Mount and Unmount ask of a driver instead of +// executing mount(8), so the orchestration -- cleanup ordering, state file +// lifecycle, unmount policy -- can be exercised without root. +type fakeDriver struct { + name mode + + preflightErr error + mountErr map[string]error // mountpoint -> MountLayer failure + overlayErr error + unmountErr map[string]error // mountpoint -> Unmount failure + + mounted []string // MountLayer calls, in order + overlays []overlayCall // AssembleOverlay calls, in order + unmounted []string // Unmount calls, in order + closed []string // teardown closures invoked, in order +} + +type overlayCall struct { + lowers []string + upper, work, merged string + readOnly bool +} + +func (f *fakeDriver) Name() mode { return f.name } +func (f *fakeDriver) Preflight() error { return f.preflightErr } + +func (f *fakeDriver) MountLayer(_ context.Context, _, mp string) (func() error, error) { + if err := f.mountErr[mp]; err != nil { + return nil, err + } + f.mounted = append(f.mounted, mp) + return func() error { + f.closed = append(f.closed, mp) + return nil + }, nil +} + +func (f *fakeDriver) AssembleOverlay(_ context.Context, lowers []string, upper, work, merged string, readOnly bool) (func() error, error) { + f.overlays = append(f.overlays, overlayCall{ + lowers: slices.Clone(lowers), upper: upper, work: work, merged: merged, readOnly: readOnly, + }) + if f.overlayErr != nil { + return nil, f.overlayErr + } + return func() error { + f.closed = append(f.closed, merged) + return nil + }, nil +} + +func (f *fakeDriver) Unmount(_ context.Context, mp string) error { + if err := f.unmountErr[mp]; err != nil { + return err + } + f.unmounted = append(f.unmounted, mp) + return nil +} + +// factory hands the same fake back for any mode. +func (f *fakeDriver) factory() driverFactory { + return func(mode) (driver, error) { return f, nil } +} + +func newFakeDriver() *fakeDriver { + return &fakeDriver{name: modeKernel} +} + +// ociDirWithLayers writes a fake OCI layout with n EROFS layers and returns a +// Source for it. +func ociDirWithLayers(t *testing.T, n int) (Source, string) { + t.Helper() + dir := t.TempDir() + layers := make([]fakeLayer, 0, n) + for i := range n { + l := fakeLayer{body: []byte{byte('a' + i)}} + if i < n-1 { + l.role = types.ErofsRoleOverlayLower + } + layers = append(layers, l) + } + writeFakeOCILayout(t, dir, layers) + src, err := ParseSource(dir) + if err != nil { + t.Fatalf("ParseSource(%s): %v", dir, err) + } + return src, dir +} + +func TestMountImage_MountOrderAndState(t *testing.T) { + src, _ := ociDirWithLayers(t, 3) + dest := t.TempDir() + f := newFakeDriver() + + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}); err != nil { + t.Fatalf("mountWith: %v", err) + } + + wantMounted := []string{ + filepath.Join(dest, "layers", "00"), + filepath.Join(dest, "layers", "01"), + filepath.Join(dest, "layers", "02"), + } + if !slices.Equal(f.mounted, wantMounted) { + t.Errorf("mounted:\n got %v\nwant %v", f.mounted, wantMounted) + } + + if len(f.overlays) != 1 { + t.Fatalf("got %d AssembleOverlay calls, want 1", len(f.overlays)) + } + // overlayfs lowerdir is highest-priority first, OCI order is bottom-up. + wantLowers := []string{wantMounted[2], wantMounted[1], wantMounted[0]} + if !slices.Equal(f.overlays[0].lowers, wantLowers) { + t.Errorf("lowers:\n got %v\nwant %v", f.overlays[0].lowers, wantLowers) + } + + st, err := loadState(dest) + if err != nil { + t.Fatalf("loadState: %v", err) + } + wantMounts := []string{ + filepath.Join(dest, "merged"), + wantMounted[2], wantMounted[1], wantMounted[0], + } + if !slices.Equal(st.Mounts, wantMounts) { + t.Errorf("state Mounts:\n got %v\nwant %v", st.Mounts, wantMounts) + } + if st.Dest != dest { + t.Errorf("state Dest: got %q want %q", st.Dest, dest) + } + if st.Mode != modeKernel { + t.Errorf("state Mode: got %q want %q", st.Mode, modeKernel) + } +} + +func TestMountImage_CleansUpInLIFOOnOverlayFailure(t *testing.T) { + src, _ := ociDirWithLayers(t, 3) + dest := t.TempDir() + f := newFakeDriver() + f.overlayErr = errors.New("overlay boom") + + err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}) + if err == nil { + t.Fatal("mountWith succeeded, want overlay failure") + } + + // Every completed layer mount must come back down, newest first. + wantClosed := []string{ + filepath.Join(dest, "layers", "02"), + filepath.Join(dest, "layers", "01"), + filepath.Join(dest, "layers", "00"), + } + if !slices.Equal(f.closed, wantClosed) { + t.Errorf("torn down:\n got %v\nwant %v", f.closed, wantClosed) + } + + // A failed mount must not leave a state file claiming success. + if _, err := os.Stat(statePath(dest)); !errors.Is(err, os.ErrNotExist) { + t.Errorf("state file after failure: stat err = %v, want ErrNotExist", err) + } +} + +func TestMountImage_RefusesExistingStateFile(t *testing.T) { + src, _ := ociDirWithLayers(t, 2) + dest := t.TempDir() + if err := os.WriteFile(statePath(dest), []byte("{}"), 0o600); err != nil { + t.Fatal(err) + } + f := newFakeDriver() + + err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}) + if err == nil { + t.Fatal("mountWith succeeded, want refusal to clobber an existing mount") + } + if len(f.mounted) != 0 { + t.Errorf("mounted %v, want nothing", f.mounted) + } +} + +func TestMountImage_SingleLayerReadOnlySkipsOverlay(t *testing.T) { + src, _ := ociDirWithLayers(t, 1) + dest := t.TempDir() + f := newFakeDriver() + + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64", ReadOnly: true}); err != nil { + t.Fatalf("mountWith: %v", err) + } + + merged := filepath.Join(dest, "merged") + if !slices.Equal(f.mounted, []string{merged}) { + t.Errorf("mounted %v, want the layer straight at %s", f.mounted, merged) + } + if len(f.overlays) != 0 { + t.Errorf("got %d AssembleOverlay calls, want none for a single read-only layer", len(f.overlays)) + } + st, err := loadState(dest) + if err != nil { + t.Fatalf("loadState: %v", err) + } + if !slices.Equal(st.Mounts, []string{merged}) { + t.Errorf("state Mounts: got %v want %v", st.Mounts, []string{merged}) + } +} + +func TestUnmountImage_OrderAndStateRemoval(t *testing.T) { + src, _ := ociDirWithLayers(t, 3) + dest := t.TempDir() + f := newFakeDriver() + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}); err != nil { + t.Fatalf("mountWith: %v", err) + } + recorded, err := loadState(dest) + if err != nil { + t.Fatalf("loadState: %v", err) + } + + if err := unmountWith(context.Background(), f.factory(), dest); err != nil { + t.Fatalf("unmountWith: %v", err) + } + + // Unmount order is exactly what the state file recorded: merged first, + // then layers top-down. + if !slices.Equal(f.unmounted, recorded.Mounts) { + t.Errorf("unmounted:\n got %v\nwant %v", f.unmounted, recorded.Mounts) + } + if _, err := os.Stat(statePath(dest)); !errors.Is(err, os.ErrNotExist) { + t.Errorf("state file after unmount: stat err = %v, want ErrNotExist", err) + } + for _, sub := range []string{"merged", "work", "layers"} { + if _, err := os.Stat(filepath.Join(dest, sub)); !errors.Is(err, os.ErrNotExist) { + t.Errorf("%s after unmount: stat err = %v, want ErrNotExist", sub, err) + } + } +} + +func TestUnmount_NoStateFileUnmountsDestItself(t *testing.T) { + dest := t.TempDir() + f := newFakeDriver() + + if err := unmountWith(context.Background(), f.factory(), dest); err != nil { + t.Fatalf("unmountWith: %v", err) + } + if !slices.Equal(f.unmounted, []string{dest}) { + t.Errorf("unmounted %v, want the blob mountpoint %s", f.unmounted, dest) + } +} From cf9d10a4f5cd86ac4a98b9334e04c948c3fd2904 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 19 Aug 2026 17:08:28 -0400 Subject: [PATCH 03/18] erofsmount: validate the state file before unmounting Unmount read /.apko-erofs-mount.json and handed every path it named to umount, in kernel mode as root. loadState checked only JSON shape and schema version. The state file lives inside dest, so anyone who can write there chooses its contents -- and the docs suggest shared destinations like /mnt/apko-erofs. A planted file naming /home had root unmount /home. Validate in loadState, so every caller is covered: - the recorded Dest must equal the dest being unmounted. Without this, a file that is internally consistent about some other directory still passes a containment check. - every entry in Mounts must be absolute and must be a path a mount actually creates under dest: /merged or /layers/NN. The second check is deliberately a whitelist rather than plain containment. Containment alone would still accept /upper or /anything, and mounts are only ever made at those two shapes, so nothing legitimate is rejected. It also subsumes traversal: a path that climbs out with .. cannot match either shape. An empty Mounts list is rejected too. Mount never writes one, and Unmount removes the file rather than emptying it. Tests cover eight rejected shapes -- absolute elsewhere, dest's parent, traversal through dest and through layers/, a relative path, dest itself, an unexpected subdir, a non-numeric layer name -- plus the dest mismatch, the empty list, and a legitimate layout being accepted. The one that states the actual guarantee is in mount_linux_test.go: a planted state file makes Unmount fail without a single path reaching the driver, rather than being noticed afterwards. All of these fail against the previous code. Refs #2408 Co-Authored-By: Claude Opus 5 (1M context) --- docs/erofs.md | 2 + pkg/erofsmount/mount_linux_test.go | 20 +++++++++ pkg/erofsmount/state.go | 38 ++++++++++++++++ pkg/erofsmount/state_test.go | 70 ++++++++++++++++++++++++++++++ 4 files changed, 130 insertions(+) diff --git a/docs/erofs.md b/docs/erofs.md index ce2d6ed7f..f28505438 100644 --- a/docs/erofs.md +++ b/docs/erofs.md @@ -186,6 +186,8 @@ apko erofs umount /mnt/apko-erofs If the kernel mount mode complains "unknown filesystem type 'erofs'", the kernel module is missing on your system; install it (e.g. `linux-modules-extra-$(uname -r)` on Ubuntu) or pass `--mode=fuse` to use `erofsfuse`, which does not require root and works inside CI containers that lack the kernel module. +`umount` works from `DEST/.apko-erofs-mount.json`, which `mount` wrote. It only accepts mountpoints that a mount creates under `DEST` — `DEST/merged` and `DEST/layers/NN` — so a tampered file cannot redirect a root `umount` elsewhere. Still, prefer a `DEST` only you can write to: anything else lets another user decide what your `umount` takes down within it. + ### Doing it manually A layer blob is a complete filesystem image, so mounting it needs no apko-specific tooling. For reference, `apko erofs mount` is equivalent to one of: diff --git a/pkg/erofsmount/mount_linux_test.go b/pkg/erofsmount/mount_linux_test.go index 8c2bc45c2..8b6a58bac 100644 --- a/pkg/erofsmount/mount_linux_test.go +++ b/pkg/erofsmount/mount_linux_test.go @@ -272,3 +272,23 @@ func TestUnmount_NoStateFileUnmountsDestItself(t *testing.T) { t.Errorf("unmounted %v, want the blob mountpoint %s", f.unmounted, dest) } } + +func TestUnmount_PlantedStateFileNeverReachesUmount(t *testing.T) { + dest := t.TempDir() + victim := t.TempDir() + planted(t, dest, dest, []string{victim}) + + f := newFakeDriver() + err := unmountWith(context.Background(), f.factory(), dest) + if err == nil { + t.Fatal("unmountWith succeeded on a state file naming a path outside dest") + } + // The point of the check: nothing is handed to umount at all, rather than + // the escape being noticed after the fact. + if len(f.unmounted) != 0 { + t.Errorf("unmounted %v; a planted state file must not reach umount", f.unmounted) + } + if _, err := os.Stat(victim); err != nil { + t.Errorf("victim dir disturbed: %v", err) + } +} diff --git a/pkg/erofsmount/state.go b/pkg/erofsmount/state.go index cdd1025e0..b199efe5c 100644 --- a/pkg/erofsmount/state.go +++ b/pkg/erofsmount/state.go @@ -21,6 +21,7 @@ import ( "io/fs" "os" "path/filepath" + "regexp" "time" ) @@ -128,9 +129,46 @@ func loadState(dest string) (*mountState, error) { if s.SchemaVersion != stateSchemaVersion { return nil, fmt.Errorf("state %s: unsupported schemaVersion %d (want %d)", path, s.SchemaVersion, stateSchemaVersion) } + if err := s.validate(dest); err != nil { + return nil, fmt.Errorf("state %s: %w", path, err) + } return &s, nil } +// mountRelPattern matches the per-layer mountpoints Mount creates, relative to +// dest. The only other one it creates is "merged". +var mountRelPattern = regexp.MustCompile(`^layers/[0-9]{2,}$`) + +// validate checks a state file before any path it names is handed to umount, +// which in kernel mode runs as root. The file lives inside dest, so anyone who +// can write there controls its contents; unvalidated, a planted file naming +// /home would have root unmount /home. +// +// Rather than only checking containment, this requires each entry to be one of +// the paths Mount actually creates. Nothing legitimate is rejected: mounts are +// made at /merged and /layers/NN and nowhere else. +func (s *mountState) validate(dest string) error { + if s.Dest != dest { + return fmt.Errorf("records dest %q but was read from %q", s.Dest, dest) + } + if len(s.Mounts) == 0 { + return errors.New("records no mounts") + } + for _, mp := range s.Mounts { + if !filepath.IsAbs(mp) { + return fmt.Errorf("mount %q is not an absolute path", mp) + } + rel, err := filepath.Rel(dest, filepath.Clean(mp)) + if err != nil { + return fmt.Errorf("mount %q is not under %s: %w", mp, dest, err) + } + if rel != "merged" && !mountRelPattern.MatchString(rel) { + return fmt.Errorf("mount %q is not a path a mount creates under %s (merged, layers/NN)", mp, dest) + } + } + return nil +} + // removeState deletes statePath(dest). It is a no-op if the file is already // absent. func removeState(dest string) error { diff --git a/pkg/erofsmount/state_test.go b/pkg/erofsmount/state_test.go index 0bfca48e6..53a2fc6ad 100644 --- a/pkg/erofsmount/state_test.go +++ b/pkg/erofsmount/state_test.go @@ -98,3 +98,73 @@ func TestLoadStateWrongSchema(t *testing.T) { t.Fatal("expected schema version error") } } + +// planted writes a state file with the given mounts directly, as an attacker +// with write access to dest would. +func planted(t *testing.T, dest string, recordedDest string, mounts []string) { + t.Helper() + if err := writeState(dest, &mountState{ + SchemaVersion: stateSchemaVersion, + Mode: modeKernel, + Dest: recordedDest, + Created: time.Date(2026, 5, 27, 12, 0, 0, 0, time.UTC), + Mounts: mounts, + }); err != nil { + t.Fatalf("writeState: %v", err) + } +} + +func TestLoadStateRejectsMountsOutsideDest(t *testing.T) { + for _, tc := range []struct { + name string + mount func(dest string) string + }{ + {"absolute elsewhere", func(string) string { return "/home" }}, + {"parent of dest", func(dest string) string { return filepath.Dir(dest) }}, + {"traversal through dest", func(dest string) string { return filepath.Join(dest, "..", "..", "etc") }}, + {"traversal through layers", func(dest string) string { return filepath.Join(dest, "layers", "..", "..", "home") }}, + {"relative", func(string) string { return "merged" }}, + {"dest itself", func(dest string) string { return dest }}, + {"unexpected subdir", func(dest string) string { return filepath.Join(dest, "upper") }}, + {"layer name not numeric", func(dest string) string { return filepath.Join(dest, "layers", "evil") }}, + } { + t.Run(tc.name, func(t *testing.T) { + dest := t.TempDir() + planted(t, dest, dest, []string{tc.mount(dest)}) + if _, err := loadState(dest); err == nil { + t.Fatalf("loadState accepted mount %q", tc.mount(dest)) + } + }) + } +} + +func TestLoadStateRejectsDestMismatch(t *testing.T) { + dest := t.TempDir() + // The recorded dest is a directory the attacker does control, while the + // mount path is inside the dest being unmounted -- so a containment-only + // check would pass this. + planted(t, dest, t.TempDir(), []string{filepath.Join(dest, "merged")}) + if _, err := loadState(dest); err == nil { + t.Fatal("loadState accepted a state file recording a different dest") + } +} + +func TestLoadStateRejectsNoMounts(t *testing.T) { + dest := t.TempDir() + planted(t, dest, dest, nil) + if _, err := loadState(dest); err == nil { + t.Fatal("loadState accepted a state file with no mounts") + } +} + +func TestLoadStateAcceptsWhatMountWrites(t *testing.T) { + dest := t.TempDir() + planted(t, dest, dest, []string{ + filepath.Join(dest, "merged"), + filepath.Join(dest, "layers", "99"), + filepath.Join(dest, "layers", "000"), + }) + if _, err := loadState(dest); err != nil { + t.Fatalf("loadState rejected a legitimate layout: %v", err) + } +} From 22c16c7d9da36d11b74ec02f714d42dc0ca2e323 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 19 Aug 2026 17:10:33 -0400 Subject: [PATCH 04/18] erofsmount: make a partial unmount recoverable On EBUSY part-way through, Unmount told the user to rerun `apko erofs umount`. That rerun could not succeed. Mounts is [merged, layers/NN..00] and the state file was never rewritten, so the second attempt started again at merged -- already unmounted -- and umount(8) exits 32 on a path that is not a mountpoint. It failed there without ever reaching the layer that was actually busy. Recovery meant unmounting by hand and deleting the state file. Drop each entry as it comes down, and on failure rewrite the state file with what is left before returning. The file then describes reality, so a rerun resumes at the entry that was busy. writeState is CreateTemp+Rename in the same directory, so a rerun never sees a half-written file. The error also names the count still up and the exact command to repeat. The fake driver now refuses to unmount a path twice, mirroring umount's exit 32. Without that a rerun would pass simply because the fake was willing to take merged down again, which is the bug this commit fixes. The new test unmounts a three-layer image with layers/01 held busy, asserts merged and layers/02 came down, asserts the state file now lists exactly [layers/01, layers/00], then clears the error and reruns: the teardown completes and the state file is gone. It fails against the previous code at the state file assertion. Refs #2408 Co-Authored-By: Claude Opus 5 (1M context) --- docs/erofs.md | 2 ++ pkg/erofsmount/mount_linux.go | 14 ++++++-- pkg/erofsmount/mount_linux_test.go | 52 ++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/docs/erofs.md b/docs/erofs.md index f28505438..a45f3fe77 100644 --- a/docs/erofs.md +++ b/docs/erofs.md @@ -188,6 +188,8 @@ If the kernel mount mode complains "unknown filesystem type 'erofs'", the kernel `umount` works from `DEST/.apko-erofs-mount.json`, which `mount` wrote. It only accepts mountpoints that a mount creates under `DEST` — `DEST/merged` and `DEST/layers/NN` — so a tampered file cannot redirect a root `umount` elsewhere. Still, prefer a `DEST` only you can write to: anything else lets another user decide what your `umount` takes down within it. +If a mountpoint is busy, `umount` stops there and rewrites the state file to list only what is still mounted, so rerunning it once the mount is free finishes the teardown. + ### Doing it manually A layer blob is a complete filesystem image, so mounting it needs no apko-specific tooling. For reference, `apko erofs mount` is equivalent to one of: diff --git a/pkg/erofsmount/mount_linux.go b/pkg/erofsmount/mount_linux.go index 6230de924..1bed5072a 100644 --- a/pkg/erofsmount/mount_linux.go +++ b/pkg/erofsmount/mount_linux.go @@ -236,10 +236,20 @@ func unmountImage(ctx context.Context, newDrv driverFactory, dest string, st *mo // would also leave the state file out of sync with reality. The user // can rerun `apko erofs umount` after addressing whatever is keeping // the mount busy. - for _, mp := range st.Mounts { + // Drop each entry as it comes down and rewrite the state file if one + // fails, so the file always describes what is still mounted. Without that + // a rerun starts again at merged -- already gone -- and fails there + // without ever reaching the entry that was busy. + for len(st.Mounts) > 0 { + mp := st.Mounts[0] if err := drv.Unmount(ctx, mp); err != nil { - return fmt.Errorf("umount %s: %w (remaining mounts left intact; rerun once they are no longer busy)", mp, err) + if werr := writeState(dest, st); werr != nil { + log.Warnf("rewrite state after partial unmount: %v", werr) + } + return fmt.Errorf("umount %s: %w (%d mount(s) still up; rerun `apko erofs umount %s` once they are no longer busy)", + mp, err, len(st.Mounts), dest) } + st.Mounts = st.Mounts[1:] log.Infof("unmounted %s", mp) } for _, sub := range []string{"merged", "upper", "work", "layers"} { diff --git a/pkg/erofsmount/mount_linux_test.go b/pkg/erofsmount/mount_linux_test.go index 8b6a58bac..66efbdc4b 100644 --- a/pkg/erofsmount/mount_linux_test.go +++ b/pkg/erofsmount/mount_linux_test.go @@ -19,6 +19,7 @@ package erofsmount import ( "context" "errors" + "fmt" "os" "path/filepath" "slices" @@ -81,6 +82,12 @@ func (f *fakeDriver) Unmount(_ context.Context, mp string) error { if err := f.unmountErr[mp]; err != nil { return err } + if slices.Contains(f.unmounted, mp) { + // umount(8) exits 32 on a path that is not a mountpoint. Mirroring + // that keeps a rerun from passing just because the fake is willing to + // unmount the same path twice. + return fmt.Errorf("umount %s: not mounted", mp) + } f.unmounted = append(f.unmounted, mp) return nil } @@ -292,3 +299,48 @@ func TestUnmount_PlantedStateFileNeverReachesUmount(t *testing.T) { t.Errorf("victim dir disturbed: %v", err) } } + +func TestUnmountImage_PartialFailureIsResumable(t *testing.T) { + src, _ := ociDirWithLayers(t, 3) + dest := t.TempDir() + f := newFakeDriver() + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}); err != nil { + t.Fatalf("mountWith: %v", err) + } + + merged := filepath.Join(dest, "merged") + top := filepath.Join(dest, "layers", "02") + busy := filepath.Join(dest, "layers", "01") + base := filepath.Join(dest, "layers", "00") + + // Something still has layers/01 open. + f.unmountErr = map[string]error{busy: errors.New("target is busy")} + if err := unmountWith(context.Background(), f.factory(), dest); err == nil { + t.Fatal("unmountWith succeeded with a busy mount") + } + if want := []string{merged, top}; !slices.Equal(f.unmounted, want) { + t.Errorf("unmounted before the failure:\n got %v\nwant %v", f.unmounted, want) + } + + // The state file must now describe only what is still up, or the rerun + // below starts at merged -- already gone -- and dies there. + st, err := loadState(dest) + if err != nil { + t.Fatalf("loadState after partial unmount: %v", err) + } + if want := []string{busy, base}; !slices.Equal(st.Mounts, want) { + t.Fatalf("state Mounts after partial unmount:\n got %v\nwant %v", st.Mounts, want) + } + + // Whatever held it open lets go; the rerun finishes the job. + f.unmountErr = nil + if err := unmountWith(context.Background(), f.factory(), dest); err != nil { + t.Fatalf("rerun after the busy mount cleared: %v", err) + } + if want := []string{merged, top, busy, base}; !slices.Equal(f.unmounted, want) { + t.Errorf("unmounted overall:\n got %v\nwant %v", f.unmounted, want) + } + if _, err := os.Stat(statePath(dest)); !errors.Is(err, os.ErrNotExist) { + t.Errorf("state file after the rerun: stat err = %v, want ErrNotExist", err) + } +} From 3820ef55a117a552544e2fa0a5c8dddc5c8517bc Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 19 Aug 2026 17:14:19 -0400 Subject: [PATCH 05/18] erofsmount: mount read-only unless asked otherwise The default mount was writable, and umount then RemoveAll'd /upper with no flag and no warning, silently discarding everything written through it. Read-only fits what these commands are for -- looking at an image apko just built -- so writes become opt-in. MountOptions.ReadOnly becomes MountOptions.Writable, so the zero value is the safe one and a library caller that fills in nothing gets a read-only mount. The CLI's --read-only becomes --rw. upper and work are only created when writable; overlayfs takes a lowerdir-only stack otherwise. A single-layer image now skips overlayfs by default rather than only when asked, which is every image apko can currently produce. umount no longer removes upper at all. For a read-only mount there is nothing there; for a writable one it is the only copy of what was written, so it is left behind and its path logged. merged, work and layers still go. mountState gains a writable field so umount knows which case it is in. No schema bump: mount/umount has never been in a release, so there are no v1 state files to stay compatible with, and the field's zero value reads as read-only anyway. Two tests: a two-layer default mount asserts the overlay is assembled read-only, upper and work are absent, and the state file agrees; the --rw case asserts the overlay is writable, writes a file into upper, unmounts, and requires the file to still be there afterwards. Refs #2408 Co-Authored-By: Claude Opus 5 (1M context) --- docs/erofs.md | 4 +- internal/cli/erofs.go | 19 ++++--- pkg/erofsmount/mount_linux.go | 23 +++++++-- pkg/erofsmount/mount_linux_test.go | 79 +++++++++++++++++++++++++++++- pkg/erofsmount/state.go | 11 +++-- 5 files changed, 119 insertions(+), 17 deletions(-) diff --git a/docs/erofs.md b/docs/erofs.md index a45f3fe77..8c6d4333e 100644 --- a/docs/erofs.md +++ b/docs/erofs.md @@ -174,7 +174,9 @@ The merge approximates what the kernel would assemble, and diverges in two corne ## Mount the layer -`apko erofs mount SOURCE DEST` mounts a raw EROFS blob or an OCI image directory at `DEST`. It chooses between a kernel mount (root) and `erofsfuse` (unprivileged) based on the effective UID; use `--mode=kernel|fuse|auto` to force a choice. `--read-only` mounts the image without an upper/work overlay; for a single-layer image that means the lone layer is mounted straight at `DEST/merged` with no overlayfs in the path. `apko erofs umount DEST` tears it back down. +`apko erofs mount SOURCE DEST` mounts a raw EROFS blob or an OCI image directory at `DEST`. It chooses between a kernel mount (root) and `erofsfuse` (unprivileged) based on the effective UID; use `--mode=kernel|fuse|auto` to force a choice. `apko erofs umount DEST` tears it back down. + +The mount is **read-only** unless you pass `--rw`, which fits inspecting an image and means a single-layer image skips overlayfs entirely — the lone layer is mounted straight at `DEST/merged`. With `--rw` you get an overlayfs upperdir at `DEST/upper`, and `umount` leaves that directory in place rather than deleting whatever was written through the mount. ```sh mkdir -p /mnt/apko-erofs diff --git a/internal/cli/erofs.go b/internal/cli/erofs.go index 358e35e20..4af04c663 100644 --- a/internal/cli/erofs.go +++ b/internal/cli/erofs.go @@ -39,7 +39,7 @@ anywhere.`, func erofsMount() *cobra.Command { var mode, arch string - var readOnly bool + var writable bool cmd := &cobra.Command{ Use: "mount [flags] SOURCE DEST", Short: "Mount an EROFS blob or an EROFS OCI image at DEST", @@ -54,16 +54,19 @@ SOURCE may be: For OCI sources, DEST gets this layout: DEST/layers/00..NN one per EROFS layer (00 is base) - DEST/upper overlayfs upperdir (writable mounts only) - DEST/work overlayfs workdir (writable mounts only) + DEST/upper overlayfs upperdir (--rw only) + DEST/work overlayfs workdir (--rw only) DEST/merged the combined view DEST/.apko-erofs-mount.json state for 'apko erofs umount' -With --read-only on a single-layer image, overlayfs is skipped and the -sole layer is mounted directly at DEST/merged.`, +The mount is read-only unless --rw is given. A single-layer image +mounted read-only skips overlayfs entirely: the sole layer is mounted +directly at DEST/merged. With --rw, writes land in DEST/upper, and +'apko erofs umount' leaves that directory behind rather than deleting +what was written.`, Example: ` apko erofs mount ./out:latest /mnt/x apko erofs mount --mode=fuse ./image.erofs /mnt/y - apko erofs mount --read-only oci-dir:./out:latest /mnt/z`, + apko erofs mount --rw oci-dir:./out:latest /mnt/z`, Args: cobra.ExactArgs(2), RunE: func(cmd *cobra.Command, args []string) error { src, err := erofsmount.ParseSource(args[0]) @@ -73,13 +76,13 @@ sole layer is mounted directly at DEST/merged.`, return erofsmount.Mount(cmd.Context(), src, args[1], erofsmount.MountOptions{ Mode: mode, Arch: arch, - ReadOnly: readOnly, + Writable: writable, }) }, } cmd.Flags().StringVar(&mode, "mode", "auto", "mount mode: kernel, fuse, or auto (auto = kernel if root else fuse)") cmd.Flags().StringVar(&arch, "arch", "host", "architecture to select from a multi-arch OCI index (host = process arch)") - cmd.Flags().BoolVar(&readOnly, "read-only", false, "mount the image read-only (omits upperdir/workdir; single-layer images skip overlayfs entirely)") + cmd.Flags().BoolVar(&writable, "rw", false, "mount read-write, adding an overlayfs upperdir and workdir under DEST (default is read-only)") return cmd } diff --git a/pkg/erofsmount/mount_linux.go b/pkg/erofsmount/mount_linux.go index 1bed5072a..1f46cf3d9 100644 --- a/pkg/erofsmount/mount_linux.go +++ b/pkg/erofsmount/mount_linux.go @@ -117,7 +117,7 @@ func mountImage(ctx context.Context, drv driver, src Source, dest string, opts M // Single-layer read-only short-circuit: overlay buys nothing when there's // one lower and no upper, so mount the layer straight at DEST/merged. // Multi-layer and writable mounts still compose through overlayfs. - if opts.ReadOnly && len(layers) == 1 { + if !opts.Writable && len(layers) == 1 { merged := filepath.Join(dest, "merged") if err := ensureDir(merged); err != nil { return err @@ -137,13 +137,21 @@ func mountImage(ctx context.Context, drv driver, src Source, dest string, opts M Created: time.Now().UTC(), Mounts: []string{merged}, } + // Writable is false here by construction: this path only runs for a + // read-only mount. if err := writeState(dest, state); err != nil { return fmt.Errorf("write state: %w", err) } return nil } - for _, sub := range []string{"layers", "upper", "work", "merged"} { + // upper and work only exist for a writable mount; overlayfs takes a + // lowerdir-only stack for the read-only case. + subs := []string{"layers", "merged"} + if opts.Writable { + subs = append(subs, "upper", "work") + } + for _, sub := range subs { if err := ensureDir(filepath.Join(dest, sub)); err != nil { return err } @@ -176,7 +184,7 @@ func mountImage(ctx context.Context, drv driver, src Source, dest string, opts M upper := filepath.Join(dest, "upper") work := filepath.Join(dest, "work") merged := filepath.Join(dest, "merged") - umount, err := drv.AssembleOverlay(ctx, lowers, upper, work, merged, opts.ReadOnly) + umount, err := drv.AssembleOverlay(ctx, lowers, upper, work, merged, !opts.Writable) if err != nil { return fmt.Errorf("overlay merge into %s: %w", merged, err) } @@ -190,6 +198,7 @@ func mountImage(ctx context.Context, drv driver, src Source, dest string, opts M Source: src.Raw, Dest: dest, Created: time.Now().UTC(), + Writable: opts.Writable, Mounts: mountsLIFO, } if err := writeState(dest, state); err != nil { @@ -252,11 +261,17 @@ func unmountImage(ctx context.Context, newDrv driverFactory, dest string, st *mo st.Mounts = st.Mounts[1:] log.Infof("unmounted %s", mp) } - for _, sub := range []string{"merged", "upper", "work", "layers"} { + // upper is not in this list. For a read-only mount it was never created; + // for a writable one it holds everything written through the mount, and + // deleting it here silently discarded that. + for _, sub := range []string{"merged", "work", "layers"} { if err := os.RemoveAll(filepath.Join(dest, sub)); err != nil { log.Warnf("remove %s: %v", filepath.Join(dest, sub), err) } } + if st.Writable { + log.Infof("writes made through the mount are left in %s", filepath.Join(dest, "upper")) + } if err := removeState(dest); err != nil { return fmt.Errorf("remove state file: %w", err) } diff --git a/pkg/erofsmount/mount_linux_test.go b/pkg/erofsmount/mount_linux_test.go index 66efbdc4b..aaf9327fb 100644 --- a/pkg/erofsmount/mount_linux_test.go +++ b/pkg/erofsmount/mount_linux_test.go @@ -217,7 +217,9 @@ func TestMountImage_SingleLayerReadOnlySkipsOverlay(t *testing.T) { dest := t.TempDir() f := newFakeDriver() - if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64", ReadOnly: true}); err != nil { + // Read-only is the default, so this is the shape an apko-produced + // single-layer image gets. + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}); err != nil { t.Fatalf("mountWith: %v", err) } @@ -344,3 +346,78 @@ func TestUnmountImage_PartialFailureIsResumable(t *testing.T) { t.Errorf("state file after the rerun: stat err = %v, want ErrNotExist", err) } } + +func TestMountImage_ReadOnlyByDefault(t *testing.T) { + src, _ := ociDirWithLayers(t, 2) + dest := t.TempDir() + f := newFakeDriver() + + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}); err != nil { + t.Fatalf("mountWith: %v", err) + } + + if len(f.overlays) != 1 { + t.Fatalf("got %d AssembleOverlay calls, want 1", len(f.overlays)) + } + if !f.overlays[0].readOnly { + t.Error("overlay assembled read-write; the default must be read-only") + } + // No upperdir means nothing to create and nothing to lose on umount. + for _, sub := range []string{"upper", "work"} { + if _, err := os.Stat(filepath.Join(dest, sub)); !errors.Is(err, os.ErrNotExist) { + t.Errorf("%s exists for a read-only mount: stat err = %v", sub, err) + } + } + st, err := loadState(dest) + if err != nil { + t.Fatalf("loadState: %v", err) + } + if st.Writable { + t.Error("state records a writable mount") + } +} + +func TestMountImage_WritableKeepsUpperOnUnmount(t *testing.T) { + src, _ := ociDirWithLayers(t, 2) + dest := t.TempDir() + f := newFakeDriver() + + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64", Writable: true}); err != nil { + t.Fatalf("mountWith: %v", err) + } + if len(f.overlays) != 1 { + t.Fatalf("got %d AssembleOverlay calls, want 1", len(f.overlays)) + } + if f.overlays[0].readOnly { + t.Error("overlay assembled read-only despite Writable") + } + upper := filepath.Join(dest, "upper") + for _, sub := range []string{"upper", "work"} { + if _, err := os.Stat(filepath.Join(dest, sub)); err != nil { + t.Errorf("%s missing for a writable mount: %v", sub, err) + } + } + + // Something written through the mount lands in upper. + written := filepath.Join(upper, "written-through-the-mount") + if err := os.WriteFile(written, []byte("keep me"), 0o600); err != nil { + t.Fatal(err) + } + + if err := unmountWith(context.Background(), f.factory(), dest); err != nil { + t.Fatalf("unmountWith: %v", err) + } + + // The whole point: umount must not silently discard it. + if got, err := os.ReadFile(written); err != nil { + t.Errorf("upper contents discarded by umount: %v", err) + } else if string(got) != "keep me" { + t.Errorf("upper contents changed: got %q", got) + } + // Everything else still goes. + for _, sub := range []string{"merged", "work", "layers"} { + if _, err := os.Stat(filepath.Join(dest, sub)); !errors.Is(err, os.ErrNotExist) { + t.Errorf("%s after unmount: stat err = %v, want ErrNotExist", sub, err) + } + } +} diff --git a/pkg/erofsmount/state.go b/pkg/erofsmount/state.go index b199efe5c..c2a46a47b 100644 --- a/pkg/erofsmount/state.go +++ b/pkg/erofsmount/state.go @@ -45,9 +45,11 @@ type MountOptions struct { // Arch picks a manifest from a multi-arch OCI index. "" or "host" // means runtime.GOARCH. Arch string - // ReadOnly, when true, skips upper/work overlay dirs and produces a - // pure read-only overlay during Mount. - ReadOnly bool + // Writable, when true, gives the mount an overlayfs upperdir so it can + // be written through. The zero value is read-only, which is what + // inspecting an image wants; writes are opt-in because a writable + // mount accumulates state that has nowhere to go when it comes down. + Writable bool } // stateSchemaVersion is the current mountState JSON schema version. @@ -67,6 +69,9 @@ type mountState struct { Source string `json:"source"` // the original `spec` argument Dest string `json:"dest"` // absolute path of the mount target Created time.Time `json:"created"` // wall-clock timestamp at mount completion + // Writable records whether the mount has an upperdir, so Unmount knows + // whether /upper holds anything worth keeping. + Writable bool `json:"writable"` // Mounts lists every mountpoint produced by Mount in unmount order // (LIFO): the first element is unmounted first. For an image mount this // is [/merged, /layers/NN, ..., /layers/00]. From a5264b2f8f5114b7b80045c26a3df24ca435c5aa Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 19 Aug 2026 18:36:55 -0400 Subject: [PATCH 06/18] erofsmount: fix lint findings in tests. ociDirWithLayers returned an unused dir string (unparam); drop it. Simplify a filepath.Dir wrapper lambda to the function itself (gocritic). --- pkg/erofsmount/mount_linux_test.go | 20 ++++++++++---------- pkg/erofsmount/state_test.go | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/erofsmount/mount_linux_test.go b/pkg/erofsmount/mount_linux_test.go index aaf9327fb..3350ac567 100644 --- a/pkg/erofsmount/mount_linux_test.go +++ b/pkg/erofsmount/mount_linux_test.go @@ -103,7 +103,7 @@ func newFakeDriver() *fakeDriver { // ociDirWithLayers writes a fake OCI layout with n EROFS layers and returns a // Source for it. -func ociDirWithLayers(t *testing.T, n int) (Source, string) { +func ociDirWithLayers(t *testing.T, n int) Source { t.Helper() dir := t.TempDir() layers := make([]fakeLayer, 0, n) @@ -119,11 +119,11 @@ func ociDirWithLayers(t *testing.T, n int) (Source, string) { if err != nil { t.Fatalf("ParseSource(%s): %v", dir, err) } - return src, dir + return src } func TestMountImage_MountOrderAndState(t *testing.T) { - src, _ := ociDirWithLayers(t, 3) + src := ociDirWithLayers(t, 3) dest := t.TempDir() f := newFakeDriver() @@ -169,7 +169,7 @@ func TestMountImage_MountOrderAndState(t *testing.T) { } func TestMountImage_CleansUpInLIFOOnOverlayFailure(t *testing.T) { - src, _ := ociDirWithLayers(t, 3) + src := ociDirWithLayers(t, 3) dest := t.TempDir() f := newFakeDriver() f.overlayErr = errors.New("overlay boom") @@ -196,7 +196,7 @@ func TestMountImage_CleansUpInLIFOOnOverlayFailure(t *testing.T) { } func TestMountImage_RefusesExistingStateFile(t *testing.T) { - src, _ := ociDirWithLayers(t, 2) + src := ociDirWithLayers(t, 2) dest := t.TempDir() if err := os.WriteFile(statePath(dest), []byte("{}"), 0o600); err != nil { t.Fatal(err) @@ -213,7 +213,7 @@ func TestMountImage_RefusesExistingStateFile(t *testing.T) { } func TestMountImage_SingleLayerReadOnlySkipsOverlay(t *testing.T) { - src, _ := ociDirWithLayers(t, 1) + src := ociDirWithLayers(t, 1) dest := t.TempDir() f := newFakeDriver() @@ -240,7 +240,7 @@ func TestMountImage_SingleLayerReadOnlySkipsOverlay(t *testing.T) { } func TestUnmountImage_OrderAndStateRemoval(t *testing.T) { - src, _ := ociDirWithLayers(t, 3) + src := ociDirWithLayers(t, 3) dest := t.TempDir() f := newFakeDriver() if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}); err != nil { @@ -303,7 +303,7 @@ func TestUnmount_PlantedStateFileNeverReachesUmount(t *testing.T) { } func TestUnmountImage_PartialFailureIsResumable(t *testing.T) { - src, _ := ociDirWithLayers(t, 3) + src := ociDirWithLayers(t, 3) dest := t.TempDir() f := newFakeDriver() if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}); err != nil { @@ -348,7 +348,7 @@ func TestUnmountImage_PartialFailureIsResumable(t *testing.T) { } func TestMountImage_ReadOnlyByDefault(t *testing.T) { - src, _ := ociDirWithLayers(t, 2) + src := ociDirWithLayers(t, 2) dest := t.TempDir() f := newFakeDriver() @@ -378,7 +378,7 @@ func TestMountImage_ReadOnlyByDefault(t *testing.T) { } func TestMountImage_WritableKeepsUpperOnUnmount(t *testing.T) { - src, _ := ociDirWithLayers(t, 2) + src := ociDirWithLayers(t, 2) dest := t.TempDir() f := newFakeDriver() diff --git a/pkg/erofsmount/state_test.go b/pkg/erofsmount/state_test.go index 53a2fc6ad..da954ebd8 100644 --- a/pkg/erofsmount/state_test.go +++ b/pkg/erofsmount/state_test.go @@ -120,7 +120,7 @@ func TestLoadStateRejectsMountsOutsideDest(t *testing.T) { mount func(dest string) string }{ {"absolute elsewhere", func(string) string { return "/home" }}, - {"parent of dest", func(dest string) string { return filepath.Dir(dest) }}, + {"parent of dest", filepath.Dir}, {"traversal through dest", func(dest string) string { return filepath.Join(dest, "..", "..", "etc") }}, {"traversal through layers", func(dest string) string { return filepath.Join(dest, "layers", "..", "..", "home") }}, {"relative", func(string) string { return "merged" }}, From a1910794230727c06c72f85ca822823c2b2f7a16 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 19 Aug 2026 19:42:48 -0400 Subject: [PATCH 07/18] erofs: drive mount/umount from hack/test-erofs.sh The privileged CI job added in #2414 exercised mount(8) directly, so nothing anywhere ran the orchestration in pkg/erofsmount against a real kernel -- the unit tests drive it through a fake driver, which cannot catch a wrong mount(8) invocation or a layout that overlayfs rejects. Extend the script with four sections after the existing comparison: - Read-only image mount: assert the single-layer short-circuit (the layer straight at merged, no layers/ or upper/), check the state file's mode, dest, writable and mounts, and diff the mounted tree against the `apko erofs ls` listing already computed above. - `--rw`: assert the overlay layout (layers/00 plus merged) and the LIFO order recorded in the state file, write a file through the mount, and check umount left it in upper/ rather than deleting it. - A raw blob, which carries no state file, so umount takes the single-mountpoint fall-back. - A tampered state file naming a decoy tmpfs outside DEST: umount must fail *and* leave the decoy mounted. Asserting on the message alone would pass against code that ran the umount anyway. Cleanup no longer tracks one mountpoint. It reads /proc/self/mounts and unmounts everything under the workdir, deepest first, which covers the mounts apko makes and a mount left behind by a failure mid-test; the rm -rf now runs under sudo since apko created part of the tree as root. The two normalization pipelines become functions so the mounted-tree comparison can be reused, and the mount sections run apko through sudo via an absolute path. --- docs/erofs.md | 2 +- hack/test-erofs.sh | 204 +++++++++++++++++++++++++++++++++++++-------- 2 files changed, 171 insertions(+), 35 deletions(-) diff --git a/docs/erofs.md b/docs/erofs.md index 8c6d4333e..670e43c15 100644 --- a/docs/erofs.md +++ b/docs/erofs.md @@ -210,7 +210,7 @@ fusermount3 -u /mnt/apko-erofs # or `fusermount -u` If `mount` reports "unknown filesystem type 'erofs'", the kernel module is missing on your system; install it (e.g. `linux-modules-extra-$(uname -r)` on Ubuntu) or use `erofsfuse`, which needs no root and works inside CI containers that lack the module. -`hack/test-erofs.sh` runs everything above in one go — build, `fsck.erofs`, kernel mount, and a comparison of `apko erofs ls` against the mounted tree — and is what the `EROFS` CI workflow executes. +`hack/test-erofs.sh` runs everything above in one go — build, `fsck.erofs`, kernel mount, a comparison of `apko erofs ls` against the mounted tree, and a round trip through `apko erofs mount` and `apko erofs umount` (read-only, `--rw`, a raw blob, and a tampered state file) — and is what the `EROFS` CI workflow executes. ## Pulling from a registry diff --git a/hack/test-erofs.sh b/hack/test-erofs.sh index 7836af202..520fb540a 100755 --- a/hack/test-erofs.sh +++ b/hack/test-erofs.sh @@ -4,8 +4,9 @@ # SPDX-License-Identifier: Apache-2.0 # End-to-end check of an apko --format=erofs build: the layer blob is a real -# EROFS filesystem that erofs-utils accepts and the kernel will mount, and -# `apko erofs ls` reports the same tree the kernel does. +# EROFS filesystem that erofs-utils accepts and the kernel will mount, +# `apko erofs ls` reports the same tree the kernel does, and +# `apko erofs mount` / `apko erofs umount` drive that mount themselves. # # Usage: hack/test-erofs.sh # @@ -14,7 +15,8 @@ # # Requires: jq, erofs-utils (fsck.erofs, dump.erofs), the kernel erofs driver, # and root (or passwordless sudo) for the mount. Set APKO to use a binary -# other than ./apko. +# other than ./apko. The --rw section needs a TMPDIR that overlayfs accepts +# as an upperdir, which rules out tmpfs on older kernels. set -euo pipefail @@ -26,11 +28,15 @@ fi yaml="$1" apko="${APKO:-./apko}" name=$(basename "${yaml}" .yaml) +# Written inside DEST by `apko erofs mount`, read back by `apko erofs umount`. +state=".apko-erofs-mount.json" if [ ! -x "${apko}" ]; then echo "no apko binary at ${apko}; run 'make apko' first" >&2 exit 1 fi +# Resolved once: the mount sections run it through sudo. +apko=$(readlink -f "${apko}") for tool in jq fsck.erofs dump.erofs; do command -v "${tool}" >/dev/null || { @@ -51,15 +57,75 @@ mnt="${workdir}/mnt" out="${workdir}/out" mkdir -p "${mnt}" "${out}" -mounted="" cleanup() { # Unmount before removing anything, on the failure paths too: leaving a - # mount behind wedges the rest of the job. - [ -n "${mounted}" ] && "${sudo[@]}" umount "${mnt}" || true - rm -rf "${workdir}" + # mount behind wedges the rest of the job. Everything this script mounts + # lives under workdir, so take down whatever is still there, deepest first, + # rather than tracking each mount separately. + local mp + while read -r mp; do + "${sudo[@]}" umount "${mp}" || true + done < <(awk -v pfx="${workdir}/" 'index($2, pfx) == 1 { print $2 }' \ + /proc/self/mounts | LC_ALL=C sort -r) + # apko ran under sudo, so parts of workdir are root-owned by now. Don't + # let a failure here mask the script's own exit status. + "${sudo[@]}" rm -rf "${workdir:?}" || + echo "warning: ${workdir} not fully removed" >&2 } trap cleanup EXIT +fail() { + echo "$*" >&2 + exit 1 +} + +assert_mounted() { + mountpoint -q "$1" || fail "expected $1 to be a mountpoint" +} + +assert_not_mounted() { + if mountpoint -q "$1"; then + fail "expected $1 not to be a mountpoint" + fi +} + +assert_absent() { + [ ! -e "$1" ] || fail "expected $1 to be gone" +} + +# assert_state DEST JQ-EXPR EXPECTED. The state file is written 0600 by root, +# so it is read back through sudo. +assert_state() { + local got + got=$("${sudo[@]}" jq -r "$2" "$1/${state}") + [ "${got}" = "$3" ] || fail "state of $1: $2 is ${got}, expected $3" +} + +# Both listings below collapse to "mode uid/gid path [-> target]" so they can +# be diffed. `apko erofs ls` columns: mode uid/gid size date time path +# [-> target]. +normalize_ls() { + awk '{ + line = $1 " " $2 " " $6 + if ($7 == "->") line = line " -> " $8 + print line + }' | LC_ALL=C sort +} + +# The walk runs privileged. The image intentionally contains mode-0700 +# directories owned by other uids (root, usr/man, var/adm), which an +# unprivileged find cannot descend into; `apko erofs ls` reads the image +# directly and is not subject to that, so the two would disagree for a reason +# that has nothing to do with apko. +tree_listing() { + "${sudo[@]}" find "$1" -mindepth 1 -printf '%M\t%U/%G\t%P\t%y\t%l\n' | + awk -F'\t' '{ + line = $1 " " $2 " " $3 + if ($4 == "l") line = line " -> " $5 + print line + }' | LC_ALL=C sort +} + echo "::group::build ${name} as erofs" "${apko}" build "${yaml}" "${name}:build" "${out}/" --format=erofs --arch=host echo "::endgroup::" @@ -102,8 +168,7 @@ if ! grep -qw erofs /proc/filesystems; then fi "${sudo[@]}" mount -t erofs -o ro "${blob}" "${mnt}" -mounted=1 -mountpoint -q "${mnt}" +assert_mounted "${mnt}" echo "::endgroup::" # Cross-check apko's own reader against the kernel's: same paths, same mode @@ -120,37 +185,108 @@ if grep -qE '[[:space:]]$| +->' "${workdir}/ls.raw"; then exit 1 fi -# ls columns: mode uid/gid size date time path [-> target] -awk '{ - line = $1 " " $2 " " $6 - if ($7 == "->") line = line " -> " $8 - print line -}' "${workdir}/ls.raw" | LC_ALL=C sort >"${workdir}/from-apko" - -# The walk runs privileged. The image intentionally contains mode-0700 -# directories owned by other uids (root, usr/man, var/adm), which an -# unprivileged find cannot descend into; `apko erofs ls` reads the image -# directly and is not subject to that, so the two would disagree for a reason -# that has nothing to do with apko. -"${sudo[@]}" find "${mnt}" -mindepth 1 -printf '%M\t%U/%G\t%P\t%y\t%l\n' | - awk -F'\t' '{ - line = $1 " " $2 " " $3 - if ($4 == "l") line = line " -> " $5 - print line - }' | LC_ALL=C sort >"${workdir}/from-kernel" +normalize_ls <"${workdir}/ls.raw" >"${workdir}/from-apko" +tree_listing "${mnt}" >"${workdir}/from-kernel" if ! diff -u "${workdir}/from-kernel" "${workdir}/from-apko"; then - echo "'apko erofs ls' disagrees with the kernel about the layer contents" >&2 - exit 1 + fail "'apko erofs ls' disagrees with the kernel about the layer contents" fi echo "$(wc -l <"${workdir}/from-apko") entries agree" echo "::endgroup::" "${sudo[@]}" umount "${mnt}" -mounted="" -if mountpoint -q "${mnt}"; then - echo "${mnt} still mounted after umount" >&2 - exit 1 +assert_not_mounted "${mnt}" + +# Everything above drives mount(8) directly. The rest drives `apko erofs +# mount` and `apko erofs umount`, which is the only place their orchestration +# -- layout, state file, teardown order -- runs against a real kernel. + +echo "::group::apko erofs mount (read-only image)" +ro="${workdir}/ro" +"${sudo[@]}" "${apko}" erofs mount "${out}" "${ro}" + +# One layer read-only: overlayfs is skipped and the layer is mounted straight +# at merged, so layers/, upper/ and work/ are never created. +assert_mounted "${ro}/merged" +assert_absent "${ro}/layers" +assert_absent "${ro}/upper" + +assert_state "${ro}" .mode kernel +assert_state "${ro}" .dest "${ro}" +assert_state "${ro}" .writable false +assert_state "${ro}" '.mounts | join(",")' "${ro}/merged" + +tree_listing "${ro}/merged" >"${workdir}/from-mount" +if ! diff -u "${workdir}/from-apko" "${workdir}/from-mount"; then + fail "'apko erofs mount' exposes a different tree than 'apko erofs ls'" +fi + +"${sudo[@]}" "${apko}" erofs umount "${ro}" +assert_not_mounted "${ro}/merged" +assert_absent "${ro}/merged" +assert_absent "${ro}/${state}" +echo "::endgroup::" + +echo "::group::apko erofs mount --rw (overlay, writes preserved)" +rw="${workdir}/rw" +"${sudo[@]}" "${apko}" erofs mount --rw "${out}" "${rw}" + +# --rw always composes through overlayfs, single layer or not. +assert_mounted "${rw}/layers/00" +assert_mounted "${rw}/merged" +assert_state "${rw}" .writable true +# LIFO: merged comes down before the layer it is stacked on. +assert_state "${rw}" '.mounts | join(",")' "${rw}/merged,${rw}/layers/00" + +echo "written through the mount" | "${sudo[@]}" tee "${rw}/merged/sentinel" >/dev/null +"${sudo[@]}" "${apko}" erofs umount "${rw}" +assert_not_mounted "${rw}/merged" +assert_absent "${rw}/merged" +assert_absent "${rw}/layers" +assert_absent "${rw}/${state}" +# upper is the one directory umount must leave alone: removing it would +# silently discard everything written through the mount. +[ -f "${rw}/upper/sentinel" ] || + fail "umount discarded the writes made through a --rw mount" +echo "::endgroup::" + +echo "::group::apko erofs mount (raw blob)" +blobmnt="${workdir}/blobmnt" +"${sudo[@]}" "${apko}" erofs mount "${blob}" "${blobmnt}" +assert_mounted "${blobmnt}" +# A blob has no enclosing directory to hold state, so umount has to fall back +# to treating dest as a single mountpoint. +"${sudo[@]}" "${apko}" erofs umount "${blobmnt}" +assert_not_mounted "${blobmnt}" +echo "::endgroup::" + +echo "::group::apko erofs umount rejects a tampered state file" +# The state file lives inside DEST, so whoever can write there decides what a +# root umount is asked to take down. Plant one naming a mount outside DEST and +# check that it is refused -- and that the decoy is still mounted afterwards, +# which is the part a message-only check would miss. +decoy="${workdir}/decoy" +tampered="${workdir}/tampered" +mkdir -p "${decoy}" "${tampered}" +"${sudo[@]}" mount -t tmpfs -o size=1m tmpfs "${decoy}" +assert_mounted "${decoy}" + +jq -n --arg dest "${tampered}" --arg mp "${decoy}" '{ + schemaVersion: 1, + mode: "kernel", + source: "tampered", + dest: $dest, + created: "2026-01-01T00:00:00Z", + writable: false, + mounts: [$mp] +}' >"${tampered}/${state}" + +if "${sudo[@]}" "${apko}" erofs umount "${tampered}"; then + fail "umount accepted a state file naming a mount outside DEST" fi +assert_mounted "${decoy}" +"${sudo[@]}" umount "${decoy}" +echo "::endgroup::" -echo "PASS: ${name} erofs layer mounts and matches 'apko erofs ls'" +echo "PASS: ${name} erofs layer mounts, matches 'apko erofs ls', and round-trips" +echo " through 'apko erofs mount' / 'apko erofs umount'" From a4a656874208e0a1a8b3d3090d94fcb68a9f3eb6 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 20 Aug 2026 12:34:30 -0400 Subject: [PATCH 08/18] erofsmount: reject a mountpoint that resolves out of dest loadState's whitelist constrains the *string* each state file entry holds -- /merged or /layers/NN -- but umount(8) canonicalizes its argument before unmounting. So the escape it was meant to stop is still reachable one step out: plant /merged as a symlink to /home along with a state file that passes validate, and a root `apko erofs umount ` takes down /home anyway. Write access inside dest is the precondition for both, so the symlink costs an attacker nothing. checkResolved requires each recorded mountpoint to resolve to the path its name claims, immediately before it is handed to the driver. It compares against dest with dest's *own* symlinks resolved, so a dest legitimately reached through one -- somewhere under /var/run, say -- keeps working and only the components below it have to be real. The check sits inside the LIFO loop rather than in a pre-pass so that a missing entry still fails exactly where umount(8) would have, leaving the earlier entries unmounted and the state file rewritten. The same symlink works on the mount side, where MkdirAll is satisfied by a link to an existing directory: /merged pointing at /etc would have the layer mounted over /etc, and with the above in place Unmount would then refuse to take it down. ensureDir now lstats and insists on a real directory. None of this closes the hole, and the comment and docs say so: the check and the umount it guards are separate steps, so someone who can write in dest can still swap a directory for a symlink in between. Closing it outright wants openat2(RESOLVE_NO_SYMLINKS) and umount -c against /proc/self/fd/N, which is a much larger change. The guidance stays "use a DEST only you can write to". hack/test-erofs.sh grows the symlinked-DEST/merged case beside the existing outside-DEST one, sharing a single tmpfs decoy through a new plant_state helper; both assert the decoy is still mounted afterwards. Co-Authored-By: Claude Opus 5 (1M context) --- docs/erofs.md | 2 +- hack/test-erofs.sh | 51 +++++++++++++------- pkg/erofsmount/mount_linux.go | 39 ++++++++++----- pkg/erofsmount/mount_linux_test.go | 55 +++++++++++++++++++++ pkg/erofsmount/state.go | 29 +++++++++++ pkg/erofsmount/state_test.go | 77 ++++++++++++++++++++++++++++++ 6 files changed, 225 insertions(+), 28 deletions(-) diff --git a/docs/erofs.md b/docs/erofs.md index 670e43c15..45cec330f 100644 --- a/docs/erofs.md +++ b/docs/erofs.md @@ -188,7 +188,7 @@ apko erofs umount /mnt/apko-erofs If the kernel mount mode complains "unknown filesystem type 'erofs'", the kernel module is missing on your system; install it (e.g. `linux-modules-extra-$(uname -r)` on Ubuntu) or pass `--mode=fuse` to use `erofsfuse`, which does not require root and works inside CI containers that lack the kernel module. -`umount` works from `DEST/.apko-erofs-mount.json`, which `mount` wrote. It only accepts mountpoints that a mount creates under `DEST` — `DEST/merged` and `DEST/layers/NN` — so a tampered file cannot redirect a root `umount` elsewhere. Still, prefer a `DEST` only you can write to: anything else lets another user decide what your `umount` takes down within it. +For an OCI source, `umount` works from `DEST/.apko-erofs-mount.json`, which `mount` wrote; a raw blob has no enclosing directory to hold one, so `umount` falls back to unmounting `DEST` itself. It only accepts mountpoints that a mount creates under `DEST` — `DEST/merged` and `DEST/layers/NN` — and checks that each still resolves to itself, so a tampered file can neither name a path outside `DEST` nor reach one through a symlink. That check and the `umount` it guards are separate steps, though, so **use a `DEST` only you can write to**: anywhere else, another user can race the two and choose what your `umount` takes down. If a mountpoint is busy, `umount` stops there and rewrites the state file to list only what is still mounted, so rerunning it once the mount is free finishes the teardown. diff --git a/hack/test-erofs.sh b/hack/test-erofs.sh index 520fb540a..0f77c778f 100755 --- a/hack/test-erofs.sh +++ b/hack/test-erofs.sh @@ -101,6 +101,20 @@ assert_state() { [ "${got}" = "$3" ] || fail "state of $1: $2 is ${got}, expected $3" } +# plant_state DEST MOUNTPOINT. Writes the state file an attacker with write +# access to DEST would, naming MOUNTPOINT as the thing to take down. +plant_state() { + jq -n --arg dest "$1" --arg mp "$2" '{ + schemaVersion: 1, + mode: "kernel", + source: "tampered", + dest: $dest, + created: "2026-01-01T00:00:00Z", + writable: false, + mounts: [$mp] + }' >"$1/${state}" +} + # Both listings below collapse to "mode uid/gid path [-> target]" so they can # be diffed. `apko erofs ls` columns: mode uid/gid size date time path # [-> target]. @@ -262,29 +276,34 @@ echo "::endgroup::" echo "::group::apko erofs umount rejects a tampered state file" # The state file lives inside DEST, so whoever can write there decides what a -# root umount is asked to take down. Plant one naming a mount outside DEST and -# check that it is refused -- and that the decoy is still mounted afterwards, -# which is the part a message-only check would miss. +# root umount is asked to take down. Two shapes have to be refused: a +# mountpoint plainly outside DEST, and one named DEST/merged -- which the +# whitelist allows -- that is a symlink pointing out. umount(8) canonicalizes +# its argument, so following the second lands on the decoy just as surely as +# the first. Both check that the decoy is still mounted afterwards, which is +# the part a message-only check would miss. decoy="${workdir}/decoy" -tampered="${workdir}/tampered" -mkdir -p "${decoy}" "${tampered}" +mkdir -p "${decoy}" "${sudo[@]}" mount -t tmpfs -o size=1m tmpfs "${decoy}" assert_mounted "${decoy}" -jq -n --arg dest "${tampered}" --arg mp "${decoy}" '{ - schemaVersion: 1, - mode: "kernel", - source: "tampered", - dest: $dest, - created: "2026-01-01T00:00:00Z", - writable: false, - mounts: [$mp] -}' >"${tampered}/${state}" - -if "${sudo[@]}" "${apko}" erofs umount "${tampered}"; then +outside="${workdir}/tampered-outside" +mkdir -p "${outside}" +plant_state "${outside}" "${decoy}" +if "${sudo[@]}" "${apko}" erofs umount "${outside}"; then fail "umount accepted a state file naming a mount outside DEST" fi assert_mounted "${decoy}" + +symlinked="${workdir}/tampered-symlink" +mkdir -p "${symlinked}" +ln -s "${decoy}" "${symlinked}/merged" +plant_state "${symlinked}" "${symlinked}/merged" +if "${sudo[@]}" "${apko}" erofs umount "${symlinked}"; then + fail "umount followed a symlinked DEST/merged out of DEST" +fi +assert_mounted "${decoy}" + "${sudo[@]}" umount "${decoy}" echo "::endgroup::" diff --git a/pkg/erofsmount/mount_linux.go b/pkg/erofsmount/mount_linux.go index 1f46cf3d9..6749d9a0e 100644 --- a/pkg/erofsmount/mount_linux.go +++ b/pkg/erofsmount/mount_linux.go @@ -239,19 +239,25 @@ func unmountImage(ctx context.Context, newDrv driverFactory, dest string, st *mo if err != nil { return err } - // st.Mounts is overlay-first then per-layer mounts in LIFO order. If - // any umount fails, stop: layer mounts that come after a still-pinned - // overlay would only return EBUSY noise, and continuing past an error - // would also leave the state file out of sync with reality. The user - // can rerun `apko erofs umount` after addressing whatever is keeping - // the mount busy. - // Drop each entry as it comes down and rewrite the state file if one - // fails, so the file always describes what is still mounted. Without that - // a rerun starts again at merged -- already gone -- and fails there - // without ever reaching the entry that was busy. + // checkResolved compares each mountpoint against dest with its own + // symlinks resolved, so resolve dest once here. + realDest, err := filepath.EvalSymlinks(dest) + if err != nil { + return fmt.Errorf("resolve dest %s: %w", dest, err) + } + // st.Mounts is overlay-first then per-layer mounts in LIFO order. If any + // umount fails, stop: layer mounts that come after a still-pinned overlay + // would only return EBUSY noise. Drop each entry as it comes down and + // rewrite the state file if one fails, so the file always describes what + // is still mounted -- without that a rerun starts again at merged, already + // gone, and fails there without ever reaching the entry that was busy. for len(st.Mounts) > 0 { mp := st.Mounts[0] - if err := drv.Unmount(ctx, mp); err != nil { + err := checkResolved(dest, realDest, mp) + if err == nil { + err = drv.Unmount(ctx, mp) + } + if err != nil { if werr := writeState(dest, st); werr != nil { log.Warnf("rewrite state after partial unmount: %v", werr) } @@ -299,5 +305,16 @@ func ensureDir(path string) error { if err := os.MkdirAll(path, 0o755); err != nil { return fmt.Errorf("mkdir %s: %w", path, err) } + // MkdirAll is satisfied by a symlink to an existing directory, so a link + // planted at /merged would have the mount land wherever it points -- + // and Unmount, which requires the same path to resolve to itself, would + // then refuse to take it back down. Only a real directory will do. + fi, err := os.Lstat(path) + if err != nil { + return fmt.Errorf("stat %s: %w", path, err) + } + if !fi.IsDir() { + return fmt.Errorf("%s is a %s, not a directory; refusing to mount there", path, fi.Mode().Type()) + } return nil } diff --git a/pkg/erofsmount/mount_linux_test.go b/pkg/erofsmount/mount_linux_test.go index 3350ac567..cc6a52714 100644 --- a/pkg/erofsmount/mount_linux_test.go +++ b/pkg/erofsmount/mount_linux_test.go @@ -302,6 +302,40 @@ func TestUnmount_PlantedStateFileNeverReachesUmount(t *testing.T) { } } +// The whitelist in validate constrains the path string; this covers the other +// half, a name that passes it but resolves somewhere else. umount(8) +// canonicalizes its argument, so following the symlink would take the victim +// down instead. +func TestUnmountImage_SymlinkedMountpointNeverReachesUmount(t *testing.T) { + src := ociDirWithLayers(t, 3) + dest := t.TempDir() + f := newFakeDriver() + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}); err != nil { + t.Fatalf("mountWith: %v", err) + } + + // Stand in for an attacker with write access to dest swapping merged -- + // the first entry in the state file -- for a link elsewhere. + victim := t.TempDir() + merged := filepath.Join(dest, "merged") + if err := os.Remove(merged); err != nil { + t.Fatal(err) + } + if err := os.Symlink(victim, merged); err != nil { + t.Fatal(err) + } + + if err := unmountWith(context.Background(), f.factory(), dest); err == nil { + t.Fatal("unmountWith succeeded on a mountpoint that resolves outside dest") + } + if len(f.unmounted) != 0 { + t.Errorf("unmounted %v; a redirected mountpoint must not reach umount", f.unmounted) + } + if _, err := os.Stat(victim); err != nil { + t.Errorf("victim dir disturbed: %v", err) + } +} + func TestUnmountImage_PartialFailureIsResumable(t *testing.T) { src := ociDirWithLayers(t, 3) dest := t.TempDir() @@ -421,3 +455,24 @@ func TestMountImage_WritableKeepsUpperOnUnmount(t *testing.T) { } } } + +// The mount-side half of the symlink problem: MkdirAll accepts a link to an +// existing directory, so without a check the layer would be mounted over +// whatever it points at -- and Unmount would then refuse to take it down. +func TestMountImage_RefusesASymlinkedMountpoint(t *testing.T) { + src := ociDirWithLayers(t, 1) + dest := t.TempDir() + victim := t.TempDir() + if err := os.Symlink(victim, filepath.Join(dest, "merged")); err != nil { + t.Fatal(err) + } + + f := newFakeDriver() + err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}) + if err == nil { + t.Fatal("mountWith succeeded with merged symlinked out of dest") + } + if len(f.mounted) != 0 { + t.Errorf("mounted %v, want nothing", f.mounted) + } +} diff --git a/pkg/erofsmount/state.go b/pkg/erofsmount/state.go index c2a46a47b..dc7599b16 100644 --- a/pkg/erofsmount/state.go +++ b/pkg/erofsmount/state.go @@ -174,6 +174,35 @@ func (s *mountState) validate(dest string) error { return nil } +// checkResolved verifies that mp still resolves to the path its name claims. +// +// validate constrains the path *string* to one that Mount creates, but +// umount(8) canonicalizes its argument before unmounting, so a symlink planted +// at /merged would redirect a root umount at whatever it points to +// without the name ever looking wrong. realDest is dest with its own symlinks +// resolved, so a legitimately symlinked dest (say one under /var/run) still +// works and only the components below it have to be real. +// +// This narrows the window rather than closing it: the check and the umount that +// follows are separate steps, so someone who can write inside dest can still +// swap a directory for a symlink in between. Both halves need dest writable by +// someone other than the caller, which is why the docs say to use a dest only +// you can write to. +func checkResolved(dest, realDest, mp string) error { + rel, err := filepath.Rel(dest, mp) + if err != nil { + return fmt.Errorf("relative to %s: %w", dest, err) + } + resolved, err := filepath.EvalSymlinks(mp) + if err != nil { + return fmt.Errorf("resolve: %w", err) + } + if want := filepath.Join(realDest, rel); resolved != want { + return fmt.Errorf("resolves to %q, not %q; a path component is a symlink", resolved, want) + } + return nil +} + // removeState deletes statePath(dest). It is a no-op if the file is already // absent. func removeState(dest string) error { diff --git a/pkg/erofsmount/state_test.go b/pkg/erofsmount/state_test.go index da954ebd8..6450cb868 100644 --- a/pkg/erofsmount/state_test.go +++ b/pkg/erofsmount/state_test.go @@ -157,6 +157,83 @@ func TestLoadStateRejectsNoMounts(t *testing.T) { } } +// resolved is checkResolved with dest resolved the way unmountImage does it. +func resolved(t *testing.T, dest, mp string) error { + t.Helper() + realDest, err := filepath.EvalSymlinks(dest) + if err != nil { + t.Fatalf("EvalSymlinks(%s): %v", dest, err) + } + return checkResolved(dest, realDest, mp) +} + +func TestCheckResolvedAcceptsRealDirectories(t *testing.T) { + dest := t.TempDir() + for _, sub := range []string{"merged", "layers/00"} { + mp := filepath.Join(dest, sub) + if err := os.MkdirAll(mp, 0o755); err != nil { + t.Fatal(err) + } + if err := resolved(t, dest, mp); err != nil { + t.Errorf("checkResolved rejected the real directory %s: %v", mp, err) + } + } +} + +// The name passes validate -- it is literally /merged -- but umount(8) +// canonicalizes, so following it would take down the victim instead. +func TestCheckResolvedRejectsSymlinkedMountpoint(t *testing.T) { + dest := t.TempDir() + victim := t.TempDir() + mp := filepath.Join(dest, "merged") + if err := os.Symlink(victim, mp); err != nil { + t.Fatal(err) + } + if err := resolved(t, dest, mp); err == nil { + t.Fatalf("checkResolved accepted %s -> %s", mp, victim) + } +} + +// A symlink higher up the recorded path is the same attack one level out. +func TestCheckResolvedRejectsSymlinkedParent(t *testing.T) { + dest := t.TempDir() + victim := t.TempDir() + if err := os.Mkdir(filepath.Join(victim, "00"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(victim, filepath.Join(dest, "layers")); err != nil { + t.Fatal(err) + } + mp := filepath.Join(dest, "layers", "00") + if err := resolved(t, dest, mp); err == nil { + t.Fatalf("checkResolved accepted %s through a symlinked parent", mp) + } +} + +// A dest that is itself reached through a symlink -- /var/run/... and the like +// -- is legitimate, and the check must not reject it just because dest and its +// resolution differ. +func TestCheckResolvedToleratesSymlinkedDest(t *testing.T) { + real := t.TempDir() + link := filepath.Join(t.TempDir(), "link") + if err := os.Symlink(real, link); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(filepath.Join(real, "merged"), 0o755); err != nil { + t.Fatal(err) + } + if err := resolved(t, link, filepath.Join(link, "merged")); err != nil { + t.Errorf("checkResolved rejected a dest reached through a symlink: %v", err) + } +} + +func TestCheckResolvedRejectsMissingPath(t *testing.T) { + dest := t.TempDir() + if err := resolved(t, dest, filepath.Join(dest, "merged")); err == nil { + t.Fatal("checkResolved accepted a path that does not exist") + } +} + func TestLoadStateAcceptsWhatMountWrites(t *testing.T) { dest := t.TempDir() planted(t, dest, dest, []string{ From 143cc35dfde2714487bb6630ffde285ae2f9b496 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 20 Aug 2026 12:35:14 -0400 Subject: [PATCH 09/18] erofsmount: report both umount failures in fuse mode fuseDriver.Unmount tries kernel umount first and falls back to fusermount, but it discarded the kernel error. Unmount is also what unmountBlob uses, since a raw blob has no state file and so no recorded mode -- which makes the kernel attempt the one that failed for the interesting reason. A busy blob mount on a host without fusermount installed therefore reported "neither fusermount3 nor fusermount found in PATH" instead of "target is busy", pointing at a missing package rather than at whatever was holding the mount open. errors.Join on both failure paths, so the caller sees what each tool actually said. The test drives Unmount with PATH set to an empty directory and asserts the result names both halves. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/erofsmount/driver_linux.go | 15 ++++++++++++--- pkg/erofsmount/driver_linux_test.go | 20 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/pkg/erofsmount/driver_linux.go b/pkg/erofsmount/driver_linux.go index 905507b0f..ca64e726b 100644 --- a/pkg/erofsmount/driver_linux.go +++ b/pkg/erofsmount/driver_linux.go @@ -163,17 +163,26 @@ func (d *fuseDriver) MountLayer(ctx context.Context, blob, mp string) (func() er // mode may itself be a kernel overlay (when overlayfs over FUSE worked) or a // fuse-overlayfs mount, and per-layer mounts are erofsfuse; umount handles the // first, fusermount -u the other two. +// +// Both failures are reported. Unmount is also the fall-back for a blob mount, +// whose mode is unknown, so the kernel attempt is the one that failed for the +// interesting reason -- returning only the fusermount error turns "target is +// busy" into "neither fusermount3 nor fusermount found in PATH". func (d *fuseDriver) Unmount(ctx context.Context, mp string) error { uargs := buildKernelUmountArgs(mp) - if err := runCmd(ctx, uargs[0], uargs[1:]...); err == nil { + kerr := runCmd(ctx, uargs[0], uargs[1:]...) + if kerr == nil { return nil } fm, err := lookupFusermount() if err != nil { - return err + return errors.Join(kerr, err) } fargs := buildFusermountUmountArgs(fm, mp) - return runCmd(ctx, fargs[0], fargs[1:]...) + if ferr := runCmd(ctx, fargs[0], fargs[1:]...); ferr != nil { + return errors.Join(kerr, ferr) + } + return nil } func (d *fuseDriver) AssembleOverlay(ctx context.Context, lowers []string, upper, work, merged string, readOnly bool) (func() error, error) { diff --git a/pkg/erofsmount/driver_linux_test.go b/pkg/erofsmount/driver_linux_test.go index feaecbdb0..496dbfe1c 100644 --- a/pkg/erofsmount/driver_linux_test.go +++ b/pkg/erofsmount/driver_linux_test.go @@ -17,11 +17,31 @@ package erofsmount import ( + "context" "reflect" "strings" "testing" ) +// The kernel umount is the attempt that fails for the interesting reason +// (EBUSY, say), and Unmount is also the blob fall-back, so losing it behind +// whatever fusermount says next would misreport why the unmount failed. +func TestFuseDriverUnmountReportsBothFailures(t *testing.T) { + // An empty PATH makes both halves fail: umount is not found, and neither + // is fusermount. + t.Setenv("PATH", t.TempDir()) + + err := (&fuseDriver{}).Unmount(context.Background(), "/mnt/x") + if err == nil { + t.Fatal("Unmount succeeded with no umount or fusermount on PATH") + } + for _, want := range []string{"umount /mnt/x", "neither fusermount3 nor fusermount"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } +} + func TestBuildKernelLayerArgs(t *testing.T) { got := buildKernelLayerArgs("/blobs/abc", "/mnt/x") want := []string{"mount", "-t", "erofs", "-o", "ro", "/blobs/abc", "/mnt/x"} From a7b6d9c32fc849bad2f6b3307743f6a1e87bec77 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 20 Aug 2026 12:35:45 -0400 Subject: [PATCH 10/18] erofsmount: refuse to reuse an upperdir, drop an empty one Not deleting /upper on umount is right -- for a --rw mount it is the only copy of everything written through the overlay -- but it left the next mount at that dest to walk into it. MkdirAll is happy with an upper that already holds the previous session's writes and the overlayfs metadata they carry, possibly for an entirely different image, and stacks it over unrelated lowers. The umount after that hands both sessions back as one. Mount now refuses a --rw mount when /upper is not empty, naming the count and saying to move or remove it. Read-only mounts never touch upper, so they are unaffected. That alone would make every --rw dest single-use, so umount cleans up the empty case: os.Remove rather than os.RemoveAll, which is the whole point of the choice. os.Remove cannot delete a non-empty directory, so writes survive by construction and the guarantee cannot be undone by a later edit here; ENOTEMPTY is what logs where they were left. An upper nothing was written through goes away and the dest is immediately reusable. hack/test-erofs.sh asserts a second --rw mount over the sentinel is refused, and that a --rw round trip writing nothing leaves no upper and can be repeated. Co-Authored-By: Claude Opus 5 (1M context) --- docs/erofs.md | 2 +- hack/test-erofs.sh | 19 +++++++++-- pkg/erofsmount/mount_linux.go | 40 ++++++++++++++++++++--- pkg/erofsmount/mount_linux_test.go | 52 ++++++++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 8 deletions(-) diff --git a/docs/erofs.md b/docs/erofs.md index 45cec330f..2728a154c 100644 --- a/docs/erofs.md +++ b/docs/erofs.md @@ -176,7 +176,7 @@ The merge approximates what the kernel would assemble, and diverges in two corne `apko erofs mount SOURCE DEST` mounts a raw EROFS blob or an OCI image directory at `DEST`. It chooses between a kernel mount (root) and `erofsfuse` (unprivileged) based on the effective UID; use `--mode=kernel|fuse|auto` to force a choice. `apko erofs umount DEST` tears it back down. -The mount is **read-only** unless you pass `--rw`, which fits inspecting an image and means a single-layer image skips overlayfs entirely — the lone layer is mounted straight at `DEST/merged`. With `--rw` you get an overlayfs upperdir at `DEST/upper`, and `umount` leaves that directory in place rather than deleting whatever was written through the mount. +The mount is **read-only** unless you pass `--rw`. Read-only is what inspecting an image wants, and it lets a single-layer image skip overlayfs entirely — the lone layer is mounted straight at `DEST/merged`. With `--rw` you get an overlayfs upperdir at `DEST/upper`; `umount` removes it only if nothing was written through the mount, and otherwise leaves it where it is and logs the path. A later `--rw` mount at the same `DEST` then refuses to start until you move or remove it, rather than quietly stacking two sessions' writes — possibly from different images — on top of each other. ```sh mkdir -p /mnt/apko-erofs diff --git a/hack/test-erofs.sh b/hack/test-erofs.sh index 0f77c778f..f8de83b71 100755 --- a/hack/test-erofs.sh +++ b/hack/test-erofs.sh @@ -258,10 +258,25 @@ assert_not_mounted "${rw}/merged" assert_absent "${rw}/merged" assert_absent "${rw}/layers" assert_absent "${rw}/${state}" -# upper is the one directory umount must leave alone: removing it would -# silently discard everything written through the mount. +# upper is the one directory umount must leave alone once something has been +# written through it: removing it would silently discard those writes. [ -f "${rw}/upper/sentinel" ] || fail "umount discarded the writes made through a --rw mount" +# And because it is still there, a second --rw mount at the same DEST has to +# refuse rather than stack this session's writes under the next one. +if "${sudo[@]}" "${apko}" erofs mount --rw "${out}" "${rw}"; then + fail "--rw mount reused an upper left behind by an earlier mount" +fi +assert_not_mounted "${rw}/merged" + +# An upper nothing was written through is not worth keeping, so that round trip +# leaves DEST clean and immediately reusable. +rw2="${workdir}/rw2" +"${sudo[@]}" "${apko}" erofs mount --rw "${out}" "${rw2}" +"${sudo[@]}" "${apko}" erofs umount "${rw2}" +assert_absent "${rw2}/upper" +"${sudo[@]}" "${apko}" erofs mount --rw "${out}" "${rw2}" +"${sudo[@]}" "${apko}" erofs umount "${rw2}" echo "::endgroup::" echo "::group::apko erofs mount (raw blob)" diff --git a/pkg/erofsmount/mount_linux.go b/pkg/erofsmount/mount_linux.go index 6749d9a0e..fbab93e69 100644 --- a/pkg/erofsmount/mount_linux.go +++ b/pkg/erofsmount/mount_linux.go @@ -24,6 +24,7 @@ import ( "os" "path/filepath" "slices" + "syscall" "time" "github.com/chainguard-dev/clog" @@ -149,6 +150,9 @@ func mountImage(ctx context.Context, drv driver, src Source, dest string, opts M // lowerdir-only stack for the read-only case. subs := []string{"layers", "merged"} if opts.Writable { + if err := checkUpperUnused(filepath.Join(dest, "upper")); err != nil { + return err + } subs = append(subs, "upper", "work") } for _, sub := range subs { @@ -267,16 +271,23 @@ func unmountImage(ctx context.Context, newDrv driverFactory, dest string, st *mo st.Mounts = st.Mounts[1:] log.Infof("unmounted %s", mp) } - // upper is not in this list. For a read-only mount it was never created; - // for a writable one it holds everything written through the mount, and - // deleting it here silently discarded that. for _, sub := range []string{"merged", "work", "layers"} { if err := os.RemoveAll(filepath.Join(dest, sub)); err != nil { log.Warnf("remove %s: %v", filepath.Join(dest, sub), err) } } - if st.Writable { - log.Infof("writes made through the mount are left in %s", filepath.Join(dest, "upper")) + // upper is the exception, and os.Remove rather than os.RemoveAll is the + // whole point: it cannot delete a non-empty directory, so writes made + // through the mount survive by construction and only an upper that was + // never written to goes away. Leaving that empty directory behind would + // make the next --rw mount at this dest refuse to start. + upper := filepath.Join(dest, "upper") + switch err := os.Remove(upper); { + case err == nil, errors.Is(err, fs.ErrNotExist): + case errors.Is(err, syscall.ENOTEMPTY), errors.Is(err, syscall.EEXIST): + log.Infof("writes made through the mount are left in %s", upper) + default: + log.Warnf("remove %s: %v", upper, err) } if err := removeState(dest); err != nil { return fmt.Errorf("remove state file: %w", err) @@ -301,6 +312,25 @@ func unmountBlob(ctx context.Context, newDrv driverFactory, dest string, log *cl return nil } +// checkUpperUnused refuses to start a writable mount on top of an upperdir an +// earlier one left behind. Unmount keeps a written-through upper rather than +// discarding it, so reusing it here would stack one session's writes -- and +// the overlayfs metadata they carry, possibly for a different image -- over +// unrelated lowers, and the next umount would hand both back as one. +func checkUpperUnused(upper string) error { + ents, err := os.ReadDir(upper) + if errors.Is(err, fs.ErrNotExist) { + return nil + } + if err != nil { + return fmt.Errorf("read %s: %w", upper, err) + } + if len(ents) > 0 { + return fmt.Errorf("%s is not empty (%d entries written through an earlier --rw mount); move or remove it first", upper, len(ents)) + } + return nil +} + func ensureDir(path string) error { if err := os.MkdirAll(path, 0o755); err != nil { return fmt.Errorf("mkdir %s: %w", path, err) diff --git a/pkg/erofsmount/mount_linux_test.go b/pkg/erofsmount/mount_linux_test.go index cc6a52714..1825dd051 100644 --- a/pkg/erofsmount/mount_linux_test.go +++ b/pkg/erofsmount/mount_linux_test.go @@ -456,6 +456,29 @@ func TestMountImage_WritableKeepsUpperOnUnmount(t *testing.T) { } } +// An upper that was never written to is not worth keeping, and leaving the +// empty directory would make the next --rw mount at this dest refuse. +func TestUnmountImage_RemovesAnUnwrittenUpper(t *testing.T) { + src := ociDirWithLayers(t, 2) + dest := t.TempDir() + f := newFakeDriver() + + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64", Writable: true}); err != nil { + t.Fatalf("mountWith: %v", err) + } + if err := unmountWith(context.Background(), f.factory(), dest); err != nil { + t.Fatalf("unmountWith: %v", err) + } + + if _, err := os.Stat(filepath.Join(dest, "upper")); !errors.Is(err, os.ErrNotExist) { + t.Errorf("empty upper left behind: stat err = %v, want ErrNotExist", err) + } + // And so the dest is reusable. + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64", Writable: true}); err != nil { + t.Errorf("second --rw mount at the same dest: %v", err) + } +} + // The mount-side half of the symlink problem: MkdirAll accepts a link to an // existing directory, so without a check the layer would be mounted over // whatever it points at -- and Unmount would then refuse to take it down. @@ -476,3 +499,32 @@ func TestMountImage_RefusesASymlinkedMountpoint(t *testing.T) { t.Errorf("mounted %v, want nothing", f.mounted) } } + +// The other half of keeping a written-through upper: it must not be silently +// stacked under a second mount, whose lowers may be a different image +// entirely. +func TestMountImage_RefusesANonEmptyUpper(t *testing.T) { + src := ociDirWithLayers(t, 2) + dest := t.TempDir() + upper := filepath.Join(dest, "upper") + if err := os.Mkdir(upper, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(upper, "from-an-earlier-mount"), []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + + f := newFakeDriver() + err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64", Writable: true}) + if err == nil { + t.Fatal("mountWith succeeded on top of a non-empty upper") + } + if len(f.mounted) != 0 { + t.Errorf("mounted %v, want nothing", f.mounted) + } + + // Read-only never touches upper, so it is still allowed. + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}); err != nil { + t.Errorf("read-only mount refused because of a leftover upper: %v", err) + } +} From ad101f6bfdf213a3a3e820e8600744d87a09f417 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 20 Aug 2026 12:36:17 -0400 Subject: [PATCH 11/18] erofsmount: escape overlayfs mount option paths Every path in an overlayfs option string here is derived from the DEST the user named, and none of them were escaped. overlayfs splits the option string on "," and lowerdir on ":", and treats "\" as an escape -- ovl_next_opt and ovl_split_lowerdirs on the way in, ovl_unescape for upperdir and workdir. A "," fails loudly as an unrecognized option, but a ":" does not: it turns one lowerdir into two, which can compose a wrong stack rather than failing. escapeOverlayPath quotes all three, applied to each lowerdir entry and to upperdir and workdir, through an overlayOpts helper the kernel and fuse-overlayfs builders now share. The replacer runs in a single pass, so the backslash rule cannot re-escape the backslashes the other two introduce. The mountpoint is its own argv element rather than part of an option value, so it stays verbatim -- the test checks that too. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/erofsmount/driver_linux.go | 42 ++++++++++++++++++++++------- pkg/erofsmount/driver_linux_test.go | 29 ++++++++++++++++++++ 2 files changed, 62 insertions(+), 9 deletions(-) diff --git a/pkg/erofsmount/driver_linux.go b/pkg/erofsmount/driver_linux.go index ca64e726b..871c23468 100644 --- a/pkg/erofsmount/driver_linux.go +++ b/pkg/erofsmount/driver_linux.go @@ -238,22 +238,46 @@ func buildFusermountUmountArgs(fusermountBin, mp string) []string { return []string{fusermountBin, "-u", mp} } -func buildKernelOverlayArgs(lowers []string, upper, work, merged string, readOnly bool) []string { - opts := "lowerdir=" + strings.Join(lowers, ":") +// escapeOverlayPath quotes a path for use inside an overlayfs mount option +// value. overlayfs splits the option string on "," and lowerdir on ":", and +// treats "\" as an escape (ovl_next_opt and ovl_split_lowerdirs on the way in, +// ovl_unescape for upperdir and workdir), so all three have to be quoted. An +// unescaped ":" is the one that matters: it turns one lowerdir into two, which +// can compose a wrong stack instead of failing. +// +// These paths all derive from the DEST the user named, so they are arbitrary. +// The single-pass replacer is what keeps the backslash rule from re-escaping +// the backslashes the other two rules introduce. +func escapeOverlayPath(p string) string { + return strings.NewReplacer(`\`, `\\`, `:`, `\:`, `,`, `\,`).Replace(p) +} + +// overlayOpts builds the shared "lowerdir=...[,upperdir=...,workdir=...]" that +// both the kernel and fuse-overlayfs option strings start from. +func overlayOpts(lowers []string, upper, work string, readOnly bool) string { + escaped := make([]string, len(lowers)) + for i, l := range lowers { + escaped[i] = escapeOverlayPath(l) + } + opts := "lowerdir=" + strings.Join(escaped, ":") if !readOnly { - opts += ",upperdir=" + upper + ",workdir=" + work - } else { + opts += ",upperdir=" + escapeOverlayPath(upper) + ",workdir=" + escapeOverlayPath(work) + } + return opts +} + +func buildKernelOverlayArgs(lowers []string, upper, work, merged string, readOnly bool) []string { + opts := overlayOpts(lowers, upper, work, readOnly) + if readOnly { opts += ",ro" } + // merged is its own argv element rather than an option value, so it is + // passed through unescaped. return []string{"mount", "-t", "overlay", "-o", opts, "overlay", merged} } func buildFuseOverlayArgs(lowers []string, upper, work, merged string, readOnly bool) []string { - opts := "lowerdir=" + strings.Join(lowers, ":") - if !readOnly { - opts += ",upperdir=" + upper + ",workdir=" + work - } - return []string{"fuse-overlayfs", "-o", opts, merged} + return []string{"fuse-overlayfs", "-o", overlayOpts(lowers, upper, work, readOnly), merged} } // lookupFusermount returns the path to whichever of `fusermount3` or diff --git a/pkg/erofsmount/driver_linux_test.go b/pkg/erofsmount/driver_linux_test.go index 496dbfe1c..de0db14e5 100644 --- a/pkg/erofsmount/driver_linux_test.go +++ b/pkg/erofsmount/driver_linux_test.go @@ -109,6 +109,35 @@ func TestBuildFuseOverlayArgs(t *testing.T) { } } +// Every one of these paths comes from the DEST the user named. ':' is the one +// that can silently compose a wrong stack rather than failing, since overlayfs +// would read a single lowerdir as two. +func TestOverlayArgsEscapeSeparators(t *testing.T) { + dest := `/mnt/od:d,d\d` + lowers := []string{dest + "/layers/01", dest + "/layers/00"} + upper, work, merged := dest+"/upper", dest+"/work", dest+"/merged" + esc := `/mnt/od\:d\,d\\d` + + kernel := buildKernelOverlayArgs(lowers, upper, work, merged, false) + wantOpts := "lowerdir=" + esc + `/layers/01:` + esc + `/layers/00` + + ",upperdir=" + esc + "/upper,workdir=" + esc + "/work" + if kernel[4] != wantOpts { + t.Errorf("kernel opts:\n got %s\nwant %s", kernel[4], wantOpts) + } + // The mountpoint is its own argv element, so it must stay verbatim. + if kernel[6] != merged { + t.Errorf("mountpoint: got %s want %s", kernel[6], merged) + } + + fuse := buildFuseOverlayArgs(lowers, upper, work, merged, false) + if fuse[2] != wantOpts { + t.Errorf("fuse opts:\n got %s\nwant %s", fuse[2], wantOpts) + } + if fuse[3] != merged { + t.Errorf("mountpoint: got %s want %s", fuse[3], merged) + } +} + func TestBuildKernelUmountArgs(t *testing.T) { got := buildKernelUmountArgs("/mnt/x") want := []string{"umount", "/mnt/x"} From fb85605fdbe27fce18f0ef19bb469561d64a142c Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 20 Aug 2026 16:07:38 -0400 Subject: [PATCH 12/18] erofsmount: unmount with umount(2) and UMOUNT_NOFOLLOW Per review: checkResolved narrows the symlink window but cannot close it, because umount(8) canonicalizes its argument. Whatever the check saw, the exec that follows resolves the path again, so a symlink swapped into /merged in between is still followed -- and in kernel mode that is a root umount of wherever it points. umount(2) with UMOUNT_NOFOLLOW refuses a symlinked final component in the same syscall that unmounts, so there is no window to race at all. kernelUnmount replaces every umount(8) exec: kernelDriver.Unmount, the teardown closures from MountLayer and AssembleOverlay, and the kernel attempt fuseDriver.Unmount makes first. buildKernelUmountArgs and its argv test go with them. The errno for the symlink case is EINVAL, which is also what "not a mountpoint" returns, so the refusal is named explicitly rather than surfaced as "invalid argument". That lstat is for the message only -- the flag is what provides the guarantee, so racing it weakens nothing. checkResolved stays, with a narrower job: UMOUNT_NOFOLLOW covers the final component, and a symlinked *parent* (/layers itself) is resolved during the kernel's own path walk, so that half is still a check followed by a syscall. Its comment and docs/erofs.md now record the split rather than claiming the whole thing is closed. The unit test can only pin the wording: unmounting a symlink to a directory that is not a mountpoint fails with or without the flag, so nothing unprivileged can tell them apart. hack/test-erofs.sh can, and now does -- a symlink pointed at a live tmpfs, passed in as DEST so nothing validates it first. Without the flag that tmpfs comes down. Co-Authored-By: Claude Opus 5 (1M context) --- docs/erofs.md | 2 +- hack/test-erofs.sh | 10 +++++++ pkg/erofsmount/driver_linux.go | 43 ++++++++++++++++++++-------- pkg/erofsmount/driver_linux_test.go | 44 ++++++++++++++++++++--------- pkg/erofsmount/state.go | 24 ++++++++-------- 5 files changed, 86 insertions(+), 37 deletions(-) diff --git a/docs/erofs.md b/docs/erofs.md index 2728a154c..c839f137d 100644 --- a/docs/erofs.md +++ b/docs/erofs.md @@ -188,7 +188,7 @@ apko erofs umount /mnt/apko-erofs If the kernel mount mode complains "unknown filesystem type 'erofs'", the kernel module is missing on your system; install it (e.g. `linux-modules-extra-$(uname -r)` on Ubuntu) or pass `--mode=fuse` to use `erofsfuse`, which does not require root and works inside CI containers that lack the kernel module. -For an OCI source, `umount` works from `DEST/.apko-erofs-mount.json`, which `mount` wrote; a raw blob has no enclosing directory to hold one, so `umount` falls back to unmounting `DEST` itself. It only accepts mountpoints that a mount creates under `DEST` — `DEST/merged` and `DEST/layers/NN` — and checks that each still resolves to itself, so a tampered file can neither name a path outside `DEST` nor reach one through a symlink. That check and the `umount` it guards are separate steps, though, so **use a `DEST` only you can write to**: anywhere else, another user can race the two and choose what your `umount` takes down. +For an OCI source, `umount` works from `DEST/.apko-erofs-mount.json`, which `mount` wrote; a raw blob has no enclosing directory to hold one, so `umount` falls back to unmounting `DEST` itself. It only accepts mountpoints that a mount creates under `DEST` — `DEST/merged` and `DEST/layers/NN` — so a tampered file cannot name a path outside `DEST`. Because that is a check on the path *string*, a symlink at `DEST/merged` would otherwise redirect it anyway; kernel mode unmounts with `umount(2)` and `UMOUNT_NOFOLLOW`, which refuses that in the same syscall, with no window. A symlinked *parent* (`DEST/layers` itself) is caught by a separate check, and there the check and the unmount are two steps — so **use a `DEST` only you can write to**: anywhere else, another user can race them and choose what your `umount` takes down. If a mountpoint is busy, `umount` stops there and rewrites the state file to list only what is still mounted, so rerunning it once the mount is free finishes the teardown. diff --git a/hack/test-erofs.sh b/hack/test-erofs.sh index f8de83b71..2cafdb801 100755 --- a/hack/test-erofs.sh +++ b/hack/test-erofs.sh @@ -319,6 +319,16 @@ if "${sudo[@]}" "${apko}" erofs umount "${symlinked}"; then fi assert_mounted "${decoy}" +# The two refusals above are both decided before umount(2) is reached, so +# neither exercises UMOUNT_NOFOLLOW. This does: a symlink pointing straight at +# a live mountpoint, handed in as DEST so nothing validates it first. Without +# the flag the kernel would resolve it and take the tmpfs down. +ln -s "${decoy}" "${workdir}/decoy-link" +if "${sudo[@]}" "${apko}" erofs umount "${workdir}/decoy-link"; then + fail "umount followed a symlink to a live mountpoint" +fi +assert_mounted "${decoy}" + "${sudo[@]}" umount "${decoy}" echo "::endgroup::" diff --git a/pkg/erofsmount/driver_linux.go b/pkg/erofsmount/driver_linux.go index 871c23468..2fb355ae6 100644 --- a/pkg/erofsmount/driver_linux.go +++ b/pkg/erofsmount/driver_linux.go @@ -26,6 +26,7 @@ import ( "strings" "github.com/chainguard-dev/clog" + "golang.org/x/sys/unix" ) // driver wraps the externally-invoked mount and umount commands used by Mount. @@ -107,14 +108,12 @@ func (d *kernelDriver) MountLayer(ctx context.Context, blob, mp string) (func() return nil, err } return func() error { - uargs := buildKernelUmountArgs(mp) - return runCmd(context.Background(), uargs[0], uargs[1:]...) + return kernelUnmount(context.Background(), mp) }, nil } func (d *kernelDriver) Unmount(ctx context.Context, mp string) error { - uargs := buildKernelUmountArgs(mp) - return runCmd(ctx, uargs[0], uargs[1:]...) + return kernelUnmount(ctx, mp) } func (d *kernelDriver) AssembleOverlay(ctx context.Context, lowers []string, upper, work, merged string, readOnly bool) (func() error, error) { @@ -123,8 +122,7 @@ func (d *kernelDriver) AssembleOverlay(ctx context.Context, lowers []string, upp return nil, err } return func() error { - uargs := buildKernelUmountArgs(merged) - return runCmd(context.Background(), uargs[0], uargs[1:]...) + return kernelUnmount(context.Background(), merged) }, nil } @@ -169,8 +167,7 @@ func (d *fuseDriver) MountLayer(ctx context.Context, blob, mp string) (func() er // interesting reason -- returning only the fusermount error turns "target is // busy" into "neither fusermount3 nor fusermount found in PATH". func (d *fuseDriver) Unmount(ctx context.Context, mp string) error { - uargs := buildKernelUmountArgs(mp) - kerr := runCmd(ctx, uargs[0], uargs[1:]...) + kerr := kernelUnmount(ctx, mp) if kerr == nil { return nil } @@ -192,8 +189,7 @@ func (d *fuseDriver) AssembleOverlay(ctx context.Context, lowers []string, upper kArgs := buildKernelOverlayArgs(lowers, upper, work, merged, readOnly) if err := runCmd(ctx, kArgs[0], kArgs[1:]...); err == nil { return func() error { - uargs := buildKernelUmountArgs(merged) - return runCmd(context.Background(), uargs[0], uargs[1:]...) + return kernelUnmount(context.Background(), merged) }, nil } @@ -226,8 +222,31 @@ func buildKernelLayerArgs(blob, mp string) []string { return []string{"mount", "-t", "erofs", "-o", "ro", blob, mp} } -func buildKernelUmountArgs(mp string) []string { - return []string{"umount", mp} +// kernelUnmount unmounts mp with umount(2) rather than umount(8). +// +// UMOUNT_NOFOLLOW is the reason: it makes the kernel refuse a symlink at the +// final component atomically, which no check-then-exec can do. umount(8) +// canonicalizes its argument, so a symlink swapped into /merged after +// validation but before the exec would still be followed, and in kernel mode +// that is a root umount of whatever it points at. +// +// This covers the final component only. A symlinked *parent* -- /layers +// itself -- resolves during the syscall's own path walk, and checkResolved is +// what rejects that, with the narrower race it documents. +func kernelUnmount(ctx context.Context, mp string) error { + clog.FromContext(ctx).Debugf("umount2: %s (UMOUNT_NOFOLLOW)", mp) + err := unix.Unmount(mp, unix.UMOUNT_NOFOLLOW) + if err == nil { + return nil + } + // EINVAL covers both "not a mountpoint" and "target is a symlink", so + // name the second case rather than leaving the user with "invalid + // 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, err) } func buildFuseLayerArgs(blob, mp string) []string { diff --git a/pkg/erofsmount/driver_linux_test.go b/pkg/erofsmount/driver_linux_test.go index de0db14e5..9ac92390e 100644 --- a/pkg/erofsmount/driver_linux_test.go +++ b/pkg/erofsmount/driver_linux_test.go @@ -18,6 +18,8 @@ package erofsmount import ( "context" + "os" + "path/filepath" "reflect" "strings" "testing" @@ -27,21 +29,45 @@ import ( // (EBUSY, say), and Unmount is also the blob fall-back, so losing it behind // whatever fusermount says next would misreport why the unmount failed. func TestFuseDriverUnmountReportsBothFailures(t *testing.T) { - // An empty PATH makes both halves fail: umount is not found, and neither - // is fusermount. + // An empty PATH makes the fusermount half fail; the kernel half fails on + // its own because the path is not a mountpoint. t.Setenv("PATH", t.TempDir()) - err := (&fuseDriver{}).Unmount(context.Background(), "/mnt/x") + mp := filepath.Join(t.TempDir(), "not-a-mountpoint") + if err := os.Mkdir(mp, 0o755); err != nil { + t.Fatal(err) + } + err := (&fuseDriver{}).Unmount(context.Background(), mp) if err == nil { - t.Fatal("Unmount succeeded with no umount or fusermount on PATH") + t.Fatal("Unmount succeeded on a path that is not mounted") } - for _, want := range []string{"umount /mnt/x", "neither fusermount3 nor fusermount"} { + for _, want := range []string{"umount " + mp, "neither fusermount3 nor fusermount"} { if !strings.Contains(err.Error(), want) { t.Errorf("error %q does not mention %q", err, want) } } } +// This pins the wording, not the flag: unmounting a symlink to a directory +// that is not a mountpoint fails either way, so no unprivileged test can tell +// UMOUNT_NOFOLLOW from its absence. hack/test-erofs.sh does, by pointing a +// symlink at a live tmpfs. What is worth pinning here is that the refusal is +// named, since the raw errno for it is a bare "invalid argument". +func TestKernelUnmountRefusesASymlink(t *testing.T) { + dir := t.TempDir() + link := filepath.Join(dir, "merged") + if err := os.Symlink(t.TempDir(), link); err != nil { + t.Fatal(err) + } + err := kernelUnmount(context.Background(), link) + if err == nil { + t.Fatal("kernelUnmount followed a symlink") + } + if !strings.Contains(err.Error(), "refusing to follow a symlink") { + t.Errorf("error %q does not name the symlink refusal", err) + } +} + func TestBuildKernelLayerArgs(t *testing.T) { got := buildKernelLayerArgs("/blobs/abc", "/mnt/x") want := []string{"mount", "-t", "erofs", "-o", "ro", "/blobs/abc", "/mnt/x"} @@ -138,14 +164,6 @@ func TestOverlayArgsEscapeSeparators(t *testing.T) { } } -func TestBuildKernelUmountArgs(t *testing.T) { - got := buildKernelUmountArgs("/mnt/x") - want := []string{"umount", "/mnt/x"} - if !reflect.DeepEqual(got, want) { - t.Fatalf("got %v, want %v", got, want) - } -} - func TestBuildFusermountUmountArgs(t *testing.T) { got := buildFusermountUmountArgs("/usr/bin/fusermount3", "/mnt/x") want := []string{"/usr/bin/fusermount3", "-u", "/mnt/x"} diff --git a/pkg/erofsmount/state.go b/pkg/erofsmount/state.go index dc7599b16..82990ea38 100644 --- a/pkg/erofsmount/state.go +++ b/pkg/erofsmount/state.go @@ -176,18 +176,20 @@ func (s *mountState) validate(dest string) error { // checkResolved verifies that mp still resolves to the path its name claims. // -// validate constrains the path *string* to one that Mount creates, but -// umount(8) canonicalizes its argument before unmounting, so a symlink planted -// at /merged would redirect a root umount at whatever it points to -// without the name ever looking wrong. realDest is dest with its own symlinks -// resolved, so a legitimately symlinked dest (say one under /var/run) still -// works and only the components below it have to be real. +// validate constrains the path *string* to one that Mount creates, which says +// nothing about where that path actually leads: a symlink planted at +// /merged would redirect a root umount without the name ever looking +// wrong. realDest is dest with its own symlinks resolved, so a legitimately +// symlinked dest (say one under /var/run) still works and only the components +// below it have to be real. // -// This narrows the window rather than closing it: the check and the umount that -// follows are separate steps, so someone who can write inside dest can still -// swap a directory for a symlink in between. Both halves need dest writable by -// someone other than the caller, which is why the docs say to use a dest only -// you can write to. +// The final component is not this check's job -- kernelUnmount passes +// UMOUNT_NOFOLLOW, which refuses it in the same syscall that unmounts, with no +// window at all. What is left to this is a symlinked *parent*, /layers +// itself, which the kernel resolves during its own path walk. There the check +// and the umount are separate steps, so someone who can write inside dest can +// still swap a directory for a symlink in between; that residual is why the +// docs say to use a dest only you can write to. func checkResolved(dest, realDest, mp string) error { rel, err := filepath.Rel(dest, mp) if err != nil { From 7f28264b1147f2a227524c97ebe0078ca0037cef Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 20 Aug 2026 16:07:59 -0400 Subject: [PATCH 13/18] erofsmount: claim dest with an exclusive state file mountImage stat'd the state file and then, several steps later, wrote it. Calling that "refuse to clobber an existing mount" overstated it: two mounts aimed at one dest both see it free and both proceed, and the second overwrites the first's record, stranding its mounts with nothing left that names them. claimState creates the file empty with O_CREATE|O_EXCL before anything is mounted, and writeState's rename replaces it with the real record at the end. The claim is registered as a cleanup, so every error path releases it. O_EXCL also declines to follow a symlink parked at that path, which the plain create would have. That leaves a window where a killed mount leaves an empty file behind, so loadState now names it -- "a mount was interrupted before it finished" beats a JSON syntax error for someone deciding what to do about it. Also from review: validate rejects a mountpoint listed twice. A repeat is inside dest so it escapes nothing, but the second umount of it fails on a path that is no longer a mountpoint, wedging the teardown on an entry that is already done. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/erofsmount/mount_linux.go | 18 ++++++++------ pkg/erofsmount/state.go | 30 +++++++++++++++++++++++ pkg/erofsmount/state_test.go | 46 +++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/pkg/erofsmount/mount_linux.go b/pkg/erofsmount/mount_linux.go index fbab93e69..1acb73e39 100644 --- a/pkg/erofsmount/mount_linux.go +++ b/pkg/erofsmount/mount_linux.go @@ -96,13 +96,6 @@ func mountImage(ctx context.Context, drv driver, src Source, dest string, opts M return err } - // Refuse to clobber an existing mount. - if _, err := os.Stat(statePath(dest)); err == nil { - return fmt.Errorf("dest %s already has a mount state file (%s); umount first", dest, statePath(dest)) - } else if !errors.Is(err, fs.ErrNotExist) { - return fmt.Errorf("stat state file: %w", err) - } - var cleanups []func() error defer func() { if retErr == nil { @@ -115,6 +108,17 @@ func mountImage(ctx context.Context, drv driver, src Source, dest string, opts M } }() + // Claim dest before mounting anything. An empty state file goes down + // first and writeState replaces it at the end, so a second mount racing + // for the same dest loses here rather than both of them proceeding. + if err := ensureDir(dest); err != nil { + return err + } + if err := claimState(dest); err != nil { + return err + } + cleanups = append(cleanups, func() error { return removeState(dest) }) + // Single-layer read-only short-circuit: overlay buys nothing when there's // one lower and no upper, so mount the layer straight at DEST/merged. // Multi-layer and writable mounts still compose through overlayfs. diff --git a/pkg/erofsmount/state.go b/pkg/erofsmount/state.go index 82990ea38..7da8bc513 100644 --- a/pkg/erofsmount/state.go +++ b/pkg/erofsmount/state.go @@ -15,6 +15,7 @@ package erofsmount import ( + "bytes" "encoding/json" "errors" "fmt" @@ -127,6 +128,9 @@ func loadState(dest string) (*mountState, error) { } return nil, fmt.Errorf("read state %s: %w", path, err) } + if len(bytes.TrimSpace(data)) == 0 { + return nil, fmt.Errorf("state %s is empty: a mount was interrupted before it finished; remove the file and clean up by hand", path) + } var s mountState if err := json.Unmarshal(data, &s); err != nil { return nil, fmt.Errorf("parse state %s: %w", path, err) @@ -159,10 +163,18 @@ func (s *mountState) validate(dest string) error { if len(s.Mounts) == 0 { return errors.New("records no mounts") } + seen := make(map[string]bool, len(s.Mounts)) for _, mp := range s.Mounts { if !filepath.IsAbs(mp) { return fmt.Errorf("mount %q is not an absolute path", mp) } + // A repeat would be unmounted twice; the second attempt fails because + // the path is no longer a mountpoint, which wedges the teardown at an + // entry that is already done. + if seen[mp] { + return fmt.Errorf("mount %q is listed more than once", mp) + } + seen[mp] = true rel, err := filepath.Rel(dest, filepath.Clean(mp)) if err != nil { return fmt.Errorf("mount %q is not under %s: %w", mp, dest, err) @@ -205,6 +217,24 @@ func checkResolved(dest, realDest, mp string) error { return nil } +// claimState reserves dest by creating the state file empty and exclusively, +// before anything is mounted; writeState replaces it with the real record once +// the mount is complete. O_EXCL rather than a stat first is what makes it a +// claim: two mounts racing for the same dest cannot both decide it is free. +// It also refuses a symlink sitting at that path, which O_CREATE alone would +// have followed. +func claimState(dest string) error { + path := statePath(dest) + f, err := os.OpenFile(path, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + if err != nil { + if errors.Is(err, fs.ErrExist) { + return fmt.Errorf("dest %s already has a mount state file (%s); umount first", dest, path) + } + return fmt.Errorf("create state file %s: %w", path, err) + } + return f.Close() +} + // removeState deletes statePath(dest). It is a no-op if the file is already // absent. func removeState(dest string) error { diff --git a/pkg/erofsmount/state_test.go b/pkg/erofsmount/state_test.go index 6450cb868..64bb82b5e 100644 --- a/pkg/erofsmount/state_test.go +++ b/pkg/erofsmount/state_test.go @@ -20,6 +20,7 @@ import ( "os" "path/filepath" "reflect" + "strings" "testing" "time" ) @@ -234,6 +235,51 @@ func TestCheckResolvedRejectsMissingPath(t *testing.T) { } } +// A repeat is inside dest, so it escapes nothing -- but the second umount of +// it fails on a path that is no longer a mountpoint, wedging the teardown at +// an entry that is already done. +func TestLoadStateRejectsDuplicateMounts(t *testing.T) { + dest := t.TempDir() + merged := filepath.Join(dest, "merged") + planted(t, dest, dest, []string{merged, merged}) + if _, err := loadState(dest); err == nil { + t.Fatal("loadState accepted a mount listed twice") + } +} + +// claimState leaves an empty file behind if the mount dies before writeState; +// say so rather than reporting a JSON syntax error. +func TestLoadStateRejectsAnEmptyFile(t *testing.T) { + dest := t.TempDir() + if err := claimState(dest); err != nil { + t.Fatal(err) + } + _, err := loadState(dest) + if err == nil { + t.Fatal("loadState accepted an empty state file") + } + if !strings.Contains(err.Error(), "interrupted") { + t.Errorf("error %q does not explain the empty file", err) + } +} + +func TestClaimStateIsExclusive(t *testing.T) { + dest := t.TempDir() + if err := claimState(dest); err != nil { + t.Fatalf("first claim: %v", err) + } + if err := claimState(dest); err == nil { + t.Fatal("second claim succeeded; the first must win the dest") + } + // And the claim is what a later writeState replaces. + if err := removeState(dest); err != nil { + t.Fatal(err) + } + if err := claimState(dest); err != nil { + t.Fatalf("claim after release: %v", err) + } +} + func TestLoadStateAcceptsWhatMountWrites(t *testing.T) { dest := t.TempDir() planted(t, dest, dest, []string{ From 4d1d3ac2e975d248b92b1fff2751ed2a163f7a66 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 20 Aug 2026 16:08:14 -0400 Subject: [PATCH 14/18] erofsmount: don't offer a retry that cannot help Every failure out of the unmount loop got the same suffix: "rerun `apko erofs umount DEST` once they are no longer busy". That is the right advice for a busy mountpoint and useless for a checkResolved rejection, which no amount of waiting clears -- the path resolves somewhere else and will keep doing so until someone removes the symlink. Split the two. A rejection says what it refused and where the remaining mounts are recorded; only a real umount failure keeps the retry hint. The state rewrite both share moves into a small helper so it cannot drift between them. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/erofsmount/mount_linux.go | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/pkg/erofsmount/mount_linux.go b/pkg/erofsmount/mount_linux.go index 1acb73e39..ae7d1aa66 100644 --- a/pkg/erofsmount/mount_linux.go +++ b/pkg/erofsmount/mount_linux.go @@ -259,18 +259,25 @@ func unmountImage(ctx context.Context, newDrv driverFactory, dest string, st *mo // rewrite the state file if one fails, so the file always describes what // is still mounted -- without that a rerun starts again at merged, already // gone, and fails there without ever reaching the entry that was busy. + // Rewrite what is left before returning any failure, so the file always + // describes what is still mounted. + stopped := func(err error) error { + if werr := writeState(dest, st); werr != nil { + log.Warnf("rewrite state after partial unmount: %v", werr) + } + return err + } for len(st.Mounts) > 0 { mp := st.Mounts[0] - err := checkResolved(dest, realDest, mp) - if err == nil { - err = drv.Unmount(ctx, mp) + // A rejection here is not something rerunning can clear, so it does + // not get the "once they are no longer busy" hint below. + if err := checkResolved(dest, realDest, mp); err != nil { + return stopped(fmt.Errorf("refusing to umount %s: %w (%d mount(s) left in %s)", + mp, err, len(st.Mounts), statePath(dest))) } - if err != nil { - if werr := writeState(dest, st); werr != nil { - log.Warnf("rewrite state after partial unmount: %v", werr) - } - return fmt.Errorf("umount %s: %w (%d mount(s) still up; rerun `apko erofs umount %s` once they are no longer busy)", - mp, err, len(st.Mounts), dest) + if err := drv.Unmount(ctx, mp); err != nil { + return stopped(fmt.Errorf("umount %s: %w (%d mount(s) still up; rerun `apko erofs umount %s` once they are no longer busy)", + mp, err, len(st.Mounts), dest)) } st.Mounts = st.Mounts[1:] log.Infof("unmounted %s", mp) From 00c9edc1fe93766ea64e0e4755201da61860f4cc Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 20 Aug 2026 16:08:30 -0400 Subject: [PATCH 15/18] erofsmount: let the fake observe the mode it is asked for From review: fakeDriver's factory discarded its mode argument, so the unit layer certified the mode plumbing without ever looking at it. unmountBlob's deliberate modeFuse choice -- the kernel-then-fusermount chain that covers a blob whose mode was never recorded -- could regress to kernel and every test here would still pass, breaking only for real fuse users. Same for unmountImage ignoring st.Mode. The factory records each mode it is handed. The blob fall-back test now asserts modeFuse, and a new test walks a mount and unmount, checking that the flag decides the first and the state file decides the second. Also fills the other hole in this file: nothing covered a Preflight failure, which is what reports a missing mount(8) or a kernel mount without root. The test asserts it stops before any mount, any overlay, and before the dest claim leaves a state file behind. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/erofsmount/mount_linux_test.go | 61 +++++++++++++++++++++++++++++- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/pkg/erofsmount/mount_linux_test.go b/pkg/erofsmount/mount_linux_test.go index 1825dd051..639fac33f 100644 --- a/pkg/erofsmount/mount_linux_test.go +++ b/pkg/erofsmount/mount_linux_test.go @@ -43,6 +43,7 @@ type fakeDriver struct { overlays []overlayCall // AssembleOverlay calls, in order unmounted []string // Unmount calls, in order closed []string // teardown closures invoked, in order + modes []mode // modes the factory was asked for, in order } type overlayCall struct { @@ -92,9 +93,15 @@ func (f *fakeDriver) Unmount(_ context.Context, mp string) error { return nil } -// factory hands the same fake back for any mode. +// factory hands the same fake back for any mode, recording which one was +// asked for. Without that record nothing here could catch unmountBlob losing +// its deliberate modeFuse choice, or unmountImage ignoring the mode the state +// file names -- both would break only for real fuse users. func (f *fakeDriver) factory() driverFactory { - return func(mode) (driver, error) { return f, nil } + return func(m mode) (driver, error) { + f.modes = append(f.modes, m) + return f, nil + } } func newFakeDriver() *fakeDriver { @@ -280,6 +287,56 @@ func TestUnmount_NoStateFileUnmountsDestItself(t *testing.T) { if !slices.Equal(f.unmounted, []string{dest}) { t.Errorf("unmounted %v, want the blob mountpoint %s", f.unmounted, dest) } + // A blob records no mode, so the fall-back deliberately picks fuse: its + // Unmount is the kernel-then-fusermount chain that covers either. + if !slices.Equal(f.modes, []mode{modeFuse}) { + t.Errorf("driver modes %v, want the blob fall-back to ask for %q", f.modes, modeFuse) + } +} + +// The mode is requested twice over a mount's life -- from the flag on the way +// up, from the state file on the way down -- and neither was observed before. +func TestMountUnmount_DriverModeFollowsFlagThenState(t *testing.T) { + src := ociDirWithLayers(t, 2) + dest := t.TempDir() + f := newFakeDriver() + + if err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64", Mode: "fuse"}); err != nil { + t.Fatalf("mountWith: %v", err) + } + if !slices.Equal(f.modes, []mode{modeFuse}) { + t.Errorf("driver modes after mount: got %v want [%q]", f.modes, modeFuse) + } + + // The fake reports itself as kernel regardless, so that is what the state + // file records and what unmount must ask for. + if err := unmountWith(context.Background(), f.factory(), dest); err != nil { + t.Fatalf("unmountWith: %v", err) + } + if want := []mode{modeFuse, modeKernel}; !slices.Equal(f.modes, want) { + t.Errorf("driver modes overall: got %v want %v", f.modes, want) + } +} + +// Preflight is what reports a missing mount(8) or a non-root kernel mount, so +// nothing may happen before it has passed. +func TestMountImage_PreflightFailureStopsBeforeAnything(t *testing.T) { + src := ociDirWithLayers(t, 2) + dest := t.TempDir() + f := newFakeDriver() + f.preflightErr = errors.New("mount(8) not found") + + err := mountWith(context.Background(), f.factory(), src, dest, MountOptions{Arch: "amd64"}) + if err == nil { + t.Fatal("mountWith succeeded despite a Preflight failure") + } + if len(f.mounted) != 0 || len(f.overlays) != 0 { + t.Errorf("mounted %v / overlays %d, want nothing", f.mounted, len(f.overlays)) + } + // Including the dest claim: a failed mount must leave no state file. + if _, err := os.Stat(statePath(dest)); !errors.Is(err, os.ErrNotExist) { + t.Errorf("state file after a Preflight failure: stat err = %v, want ErrNotExist", err) + } } func TestUnmount_PlantedStateFileNeverReachesUmount(t *testing.T) { From 1cb44f109457b41237695b458604743d9afd23ca Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 20 Aug 2026 16:08:44 -0400 Subject: [PATCH 16/18] erofs: tighten the test script and drop a stale docs note hack/test-erofs.sh: - mountpoint(1) is what every assert_mounted call runs, but it was in neither the required-tool loop nor the Requires header. A box without util-linux failed at the first assertion instead of at the preflight. - The --rw teardown checked merged and layers were gone but not work, which umount removes alongside them. - Both tampered-state cases accepted any nonzero exit, which a crash before the check ever ran would also produce. Asserting the planted state file is still there -- and the symlink undisturbed -- is what says it was refused rather than that apko fell over. docs/erofs.md: the "unknown filesystem type 'erofs'" note appeared twice, once for `apko erofs mount` and once for the manual equivalents below it. The first covers both modes and names --mode=fuse; drop the second. Co-Authored-By: Claude Opus 5 (1M context) --- docs/erofs.md | 2 -- hack/test-erofs.sh | 15 ++++++++++++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/docs/erofs.md b/docs/erofs.md index c839f137d..595fc69a4 100644 --- a/docs/erofs.md +++ b/docs/erofs.md @@ -208,8 +208,6 @@ erofsfuse out/blobs/sha256/$LAYER /mnt/apko-erofs fusermount3 -u /mnt/apko-erofs # or `fusermount -u` ``` -If `mount` reports "unknown filesystem type 'erofs'", the kernel module is missing on your system; install it (e.g. `linux-modules-extra-$(uname -r)` on Ubuntu) or use `erofsfuse`, which needs no root and works inside CI containers that lack the module. - `hack/test-erofs.sh` runs everything above in one go — build, `fsck.erofs`, kernel mount, a comparison of `apko erofs ls` against the mounted tree, and a round trip through `apko erofs mount` and `apko erofs umount` (read-only, `--rw`, a raw blob, and a tampered state file) — and is what the `EROFS` CI workflow executes. ## Pulling from a registry diff --git a/hack/test-erofs.sh b/hack/test-erofs.sh index 2cafdb801..592132d4f 100755 --- a/hack/test-erofs.sh +++ b/hack/test-erofs.sh @@ -13,8 +13,8 @@ # Example: # hack/test-erofs.sh ./examples/wolfi-base.yaml # -# Requires: jq, erofs-utils (fsck.erofs, dump.erofs), the kernel erofs driver, -# and root (or passwordless sudo) for the mount. Set APKO to use a binary +# Requires: jq, mountpoint (util-linux), erofs-utils (fsck.erofs, dump.erofs), +# the kernel erofs driver, and root (or passwordless sudo) for the mount. Set APKO to use a binary # other than ./apko. The --rw section needs a TMPDIR that overlayfs accepts # as an upperdir, which rules out tmpfs on older kernels. @@ -38,7 +38,7 @@ fi # Resolved once: the mount sections run it through sudo. apko=$(readlink -f "${apko}") -for tool in jq fsck.erofs dump.erofs; do +for tool in jq mountpoint fsck.erofs dump.erofs; do command -v "${tool}" >/dev/null || { echo "missing required tool: ${tool}" >&2 exit 1 @@ -257,6 +257,7 @@ echo "written through the mount" | "${sudo[@]}" tee "${rw}/merged/sentinel" >/de assert_not_mounted "${rw}/merged" assert_absent "${rw}/merged" assert_absent "${rw}/layers" +assert_absent "${rw}/work" assert_absent "${rw}/${state}" # upper is the one directory umount must leave alone once something has been # written through it: removing it would silently discard those writes. @@ -309,6 +310,10 @@ if "${sudo[@]}" "${apko}" erofs umount "${outside}"; then fail "umount accepted a state file naming a mount outside DEST" fi assert_mounted "${decoy}" +# A nonzero exit on its own would also match a crash before the check ran. +# The state file surviving untouched is what says it was refused. +[ -f "${outside}/${state}" ] || + fail "umount removed the state file it was supposed to refuse" symlinked="${workdir}/tampered-symlink" mkdir -p "${symlinked}" @@ -318,6 +323,10 @@ if "${sudo[@]}" "${apko}" erofs umount "${symlinked}"; then fail "umount followed a symlinked DEST/merged out of DEST" fi assert_mounted "${decoy}" +[ -f "${symlinked}/${state}" ] || + fail "umount removed the state file it was supposed to refuse" +[ -L "${symlinked}/merged" ] || + fail "umount disturbed the symlink instead of refusing it" # The two refusals above are both decided before umount(2) is reached, so # neither exercises UMOUNT_NOFOLLOW. This does: a symlink pointing straight at From fe657a5a70c0d327a2c4158193426ec74b36d4bc Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Thu, 20 Aug 2026 16:12:00 -0400 Subject: [PATCH 17/18] erofsmount: stop naming the mountpoint twice kernelUnmount reports "umount : ..." and both callers prefixed it again, so the EROFS job's log came back with umount /tmp/.../decoy-link: umount /tmp/.../decoy-link: refusing to follow a symlink Drop the outer prefix at both sites; the driver already names the path. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/erofsmount/mount_linux.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/erofsmount/mount_linux.go b/pkg/erofsmount/mount_linux.go index ae7d1aa66..44173dfd6 100644 --- a/pkg/erofsmount/mount_linux.go +++ b/pkg/erofsmount/mount_linux.go @@ -276,8 +276,9 @@ func unmountImage(ctx context.Context, newDrv driverFactory, dest string, st *mo mp, err, len(st.Mounts), statePath(dest))) } if err := drv.Unmount(ctx, mp); err != nil { - return stopped(fmt.Errorf("umount %s: %w (%d mount(s) still up; rerun `apko erofs umount %s` once they are no longer busy)", - mp, err, len(st.Mounts), dest)) + // The driver's error already names mp. + return stopped(fmt.Errorf("%w (%d mount(s) still up; rerun `apko erofs umount %s` once they are no longer busy)", + err, len(st.Mounts), dest)) } st.Mounts = st.Mounts[1:] log.Infof("unmounted %s", mp) @@ -317,7 +318,8 @@ func unmountBlob(ctx context.Context, newDrv driverFactory, dest string, log *cl return err } if err := drv.Unmount(ctx, dest); err != nil { - return fmt.Errorf("umount %s: %w", dest, err) + // The driver's error already names dest. + return err } log.Infof("unmounted %s", dest) return nil From 28acdfd7b50337359faf97bdb92c0159ba73dc61 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Fri, 21 Aug 2026 11:36:41 -0400 Subject: [PATCH 18/18] erofsmount: stop requiring umount(8) for a kernel mount Left over from the switch to umount(2): Preflight still refused to mount when umount(8) was missing from PATH, though nothing execs it any more. mount(8) is the only binary the kernel driver still needs. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/erofsmount/driver_linux.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/erofsmount/driver_linux.go b/pkg/erofsmount/driver_linux.go index 2fb355ae6..1f140f2dd 100644 --- a/pkg/erofsmount/driver_linux.go +++ b/pkg/erofsmount/driver_linux.go @@ -94,10 +94,10 @@ func (kernelDriver) Preflight() error { if os.Geteuid() != 0 { return fmt.Errorf("kernel mount mode requires root (euid 0); pass --mode=fuse to use erofsfuse instead") } - for _, bin := range []string{"mount", "umount"} { - if _, err := exec.LookPath(bin); err != nil { - return fmt.Errorf("%s not found in PATH: %w", bin, err) - } + // Only mount(8) is execed; unmounting goes through umount(2) directly, + // so umount(8) is no longer a requirement. + if _, err := exec.LookPath("mount"); err != nil { + return fmt.Errorf("mount not found in PATH: %w", err) } return nil }