fix umount cleanup

.github/workflows/build.yaml

@@ -67,10 +67,10 @@ jobs:
         run: |
           make batstest
       - name: Upload code coverage
-        uses: codecov/codecov-action@v4
+        uses: codecov/codecov-action@v5
         with:
           fail_ci_if_error: true # optional (default = false)
-          files: ./coverage.txt
+          files: ./unit-coverage.txt,./integ-coverage.txt
           token: ${{ secrets.CODECOV_TOKEN }} # required
       - name: Release
         uses: softprops/action-gh-release@v1

Makefile

@@ -14,6 +14,7 @@ BATS_VERSION := v1.10.0
 STACKER = $(TOOLS_D)/bin/stacker
 STACKER_VERSION := v1.0.0
 TOOLS_D := $(ROOT)/tools
+GOCOVERDIR ?= $(ROOT)
 
 export PATH := $(TOOLS_D)/bin:$(PATH)
 
@@ -25,11 +26,13 @@ gofmt: .made-gofmt
 		{ echo "gofmt made changes: $$o" 1>&2; exit 1; }
 	@touch $@
 
-atomfs: .made-gofmt $(GO_SRC)
-	cd $(ROOT)/cmd/atomfs && go build -buildvcs=false -ldflags "$(VERSION_LDFLAGS)" -o $(ROOT)/bin/atomfs ./...
+atomfs atomfs-cover: .made-gofmt $(GO_SRC)
+	cd $(ROOT)/cmd/atomfs && go build $(BUILDCOVERFLAGS) -buildvcs=false -ldflags "$(VERSION_LDFLAGS)" -o $(ROOT)/bin/$@ ./...
+
+atomfs-cover: BUILDCOVERFLAGS=-cover
 
 gotest: $(GO_SRC)
-	go test -coverprofile=coverage.txt -ldflags "$(VERSION_LDFLAGS)"  ./...
+	go test -coverprofile=unit-coverage.txt -ldflags "$(VERSION_LDFLAGS)"  ./...
 
 $(STACKER):
 	mkdir -p $(TOOLS_D)/bin
@@ -46,9 +49,21 @@ $(BATS):
 	git clone --depth 1 https://github.com/bats-core/bats-assert $(ROOT)/test/test_helper/bats-assert
 	git clone --depth 1 https://github.com/bats-core/bats-file $(ROOT)/test/test_helper/bats-file
 
-batstest: $(BATS) $(STACKER) atomfs test/random.txt
-	cd $(ROOT)/test; sudo $(BATS) --tap --timing priv-*.bats
-	cd $(ROOT)/test; $(BATS) --tap --timing unpriv-*.bats
+batstest: $(BATS) $(STACKER) atomfs-cover test/random.txt testimages
+	cd $(ROOT)/test; sudo GOCOVERDIR=$(GOCOVERDIR) $(BATS) --tap --timing priv-*.bats
+	cd $(ROOT)/test; GOCOVERDIR=$(GOCOVERDIR) $(BATS) --tap --timing unpriv-*.bats
+	go tool covdata textfmt -i $(GOCOVERDIR) -o integ-coverage.txt
+
+covreport: gotest batstest
+	go tool cover -func=unit-coverage.txt
+	go tool cover -func=integ-coverage.txt
+
+testimages: /tmp/atomfs-test-oci/.copydone
+	@echo "busybox image exists at /tmp/atomfs-test-oci already"
+
+/tmp/atomfs-test-oci/.copydone:
+	skopeo copy docker://public.ecr.aws/docker/library/busybox:stable oci:/tmp/atomfs-test-oci:busybox
+	touch $@
 
 test/random.txt:
 	dd if=/dev/random of=/dev/stdout count=2048 | base64 > test/random.txt

cmd/atomfs/umount.go

@@ -2,9 +2,7 @@ package main
 
 import (
 	"fmt"
-	"os"
 	"path/filepath"
-	"syscall"
 
 	"github.com/urfave/cli"
 	"machinerun.io/atomfs"
@@ -39,9 +37,7 @@ func doUmount(ctx *cli.Context) error {
 	}
 
 	mountpoint := ctx.Args()[0]
-
 	var err error
-	var errs []error
 
 	if !filepath.IsAbs(mountpoint) {
 		mountpoint, err = filepath.Abs(mountpoint)
@@ -50,53 +46,5 @@ func doUmount(ctx *cli.Context) error {
 		}
 	}
 
-	// We expect the argument to be the mountpoint of the overlay
-	err = syscall.Unmount(mountpoint, 0)
-	if err != nil {
-		errs = append(errs, fmt.Errorf("Failed unmounting %s: %v", mountpoint, err))
-	}
-
-	// We expect the following in the metadir
-	//
-	// $metadir/mounts/* - the original squashfs mounts
-	// $metadir/meta/config.json
-
-	// TODO: want to know mountnsname for a target mountpoint... not for our current proc???
-	mountNSName, err := atomfs.GetMountNSName()
-	if err != nil {
-		errs = append(errs, fmt.Errorf("Failed to get mount namespace name"))
-	}
-	metadir := filepath.Join(atomfs.RuntimeDir(ctx.String("metadir")), "meta", mountNSName, atomfs.ReplacePathSeparators(mountpoint))
-
-	mountsdir := filepath.Join(metadir, "mounts")
-	mounts, err := os.ReadDir(mountsdir)
-	if err != nil {
-		errs = append(errs, fmt.Errorf("Failed reading list of mounts: %v", err))
-		return fmt.Errorf("Encountered errors: %v", errs)
-	}
-
-	for _, m := range mounts {
-		p := filepath.Join(mountsdir, m.Name())
-		if !m.IsDir() || !isMountpoint(p) {
-			continue
-		}
-
-		err = syscall.Unmount(p, 0)
-		if err != nil {
-			errs = append(errs, fmt.Errorf("Failed unmounting squashfs dir %s: %v", p, err))
-		}
-	}
-
-	if len(errs) != 0 {
-		for i, e := range errs {
-			fmt.Printf("Error %d: %v\n", i, e)
-		}
-		return fmt.Errorf("Encountered errors %d: %v", len(errs), errs)
-	}
-
-	if err := os.RemoveAll(metadir); err != nil {
-		return fmt.Errorf("Failed removing %q: %v", metadir, err)
-	}
-
-	return nil
+	return atomfs.UmountWithMetadir(mountpoint, ctx.String("metadir"))
 }

molecule.go

@@ -287,14 +287,32 @@
 	return nil
 }
 
+// Default Umount passes "" and uses /run/atomfs metadir, see RuntimeDir().
 func Umount(dest string) error {
+	return UmountWithMetadir(dest, "")
+}
+
+func UmountWithMetadir(dest, metadirArg string) error {
 	var err error
 	dest, err = filepath.Abs(dest)
 	if err != nil {
 		return errors.Wrapf(err, "couldn't create abs path for %v", dest)
 	}
 
-	lockfile, err := makeLock(dest)
+	// recreate molecule config as much as possible, for MetadataPath():
+	mol := Molecule{
+		config: MountOCIOpts{
+			Target:      dest,
+			MetadataDir: metadirArg,
+		},
+	}
+
+	metadir, err := mol.MetadataPath()
+	if err != nil {
+		return errors.WithStack(err)
+	}
+
+	lockfile, err := makeLock(metadir)
 	if err != nil {
 		return errors.WithStack(err)
 	}
@@ -310,77 +328,93 @@
 		return err
 	}
 
-	underlyingAtoms := []string{}
+	// Find all mountpoints underlying the current top Overlay MP
+	underlyingAtomRelPaths := []string{}
 	for _, m := range mounts {
 		if m.FSType != "overlay" {
 			continue
 		}
 
-		if m.Target != dest { // TODO is this still right
+		if m.Target != dest {
 			continue
 		}
 
-		underlyingAtoms, err = m.GetOverlayDirs()
+		underlyingAtomRelPaths, err = m.GetOverlayDirs()
 		if err != nil {
 			return err
 		}
 		break
 	}
 
+	underlyingAtoms := []string{}
+	// Ensure abs paths, as we compare it to the abs path of dest
+	for _, m := range underlyingAtomRelPaths {
+		abspath, err := filepath.Abs(m)
+		if err != nil {
+			return errors.Wrapf(err, "failed to get abs path for %q", m)
+		}
+		underlyingAtoms = append(underlyingAtoms, abspath)
+	}
+
 	if len(underlyingAtoms) == 0 {
 		return errors.Errorf("%s is not an atomfs mountpoint", dest)
 	}
 
+	// Unmount the top Overlay MP
 	if err := unix.Unmount(dest, 0); err != nil {
-		return err
+		return errors.Wrapf(err, "failed to unmount dest, %q", dest)
 	}
 
-	// now, "refcount" the remaining atoms and see if any of ours are
-	// unused
-	usedAtoms := map[string]int{}
-
-	mounts, err = mount.ParseMounts("/proc/self/mountinfo")
-	if err != nil {
-		return err
-	}
+	// For each underlying dir, we need to find the corresponding
+	// squashfs-verity device mounted on it, then unconditionally unmount the
+	// underlying dir (because its mount point is specific to `dest`) - then
+	// find the underlying device and optionally clean it up too
+	for _, a := range underlyingAtoms {
+		// the workaround dir isn't really a mountpoint, so don't unmount it
+		if path.Base(a) == "workaround" {
+			continue
+		}
 
-	for _, m := range mounts {
-		if m.FSType != "overlay" {
+		// overlaydirs includes the top level mount, ignore it
+		if a == dest {
 			continue
 		}
 
-		dirs, err := m.GetOverlayDirs()
+		backingDevice, err := squashfs.GetBackingDevice(a)
 		if err != nil {
 			return err
 		}
-		for _, d := range dirs {
-			usedAtoms[d]++
+		log.Debugf("Unmounting underlying atom =%q", a)
+		if err := unix.Unmount(a, 0); err != nil {
+			return err
 		}
-	}
 
-	// If any of the atoms underlying the target mountpoint are now unused,
-	// let's unmount them too.
-	for _, a := range underlyingAtoms {
-		_, used := usedAtoms[a]
-		if used {
-			continue
+		// if that was the last mountpoint for the dev, we can clean it up too
+		mounts, err = mount.ParseMounts("/proc/self/mountinfo")
+		if err != nil {
+			return err
 		}
-		/* TODO: some kind of logging
-		if !used {
-			log.Warnf("unused atom %s was part of this molecule?")
-			continue
+		backingDevIsUsed := false
+		for _, m := range mounts {
+			if m.Source == backingDevice {
+				backingDevIsUsed = true
+			}
 		}
-		*/
 
-		// the workaround dir isn't really a mountpoint, so don't unmount it
-		if path.Base(a) == "workaround" {
-			continue
+		if !backingDevIsUsed {
+			if err := squashfs.MaybeCleanupBackingDevice(backingDevice); err != nil {
+				return err
+			}
 		}
+	}
 
-		err = squashfs.Umount(a)
-		if err != nil {
-			return err
-		}
+	mountNSName, err := GetMountNSName()
+	if err != nil {
+		return err
+	}
+	destMetaDir := filepath.Join(RuntimeDir(metadir), "meta", mountNSName, ReplacePathSeparators(dest))
+	if err := os.RemoveAll(destMetaDir); err != nil {
+		return err
 	}
 
 	return nil

squashfs/verity.go

@@ -449,25 +449,42 @@
 	return deviceNo, nil
 }
 
+// unmounts a squash mountpoint and detaches any verity / loop devices that back
+// it. Only use this if you are sure the underlying devices aren't in use by
+// other mount points.
 func Umount(mountpoint string) error {
+	devPath, err := GetBackingDevice(mountpoint)
+
+	err = unix.Unmount(mountpoint, 0)
+	if err != nil {
+		return errors.Wrapf(err, "failed unmounting %v", mountpoint)
+	}
+
+	return MaybeCleanupBackingDevice(devPath)
+}
+
+func GetBackingDevice(mountpoint string) (string, error) {
 	mounts, err := mount.ParseMounts("/proc/self/mountinfo")
 	if err != nil {
-		return err
+		return "", err
 	}
 
-	// first, find the verity device that backs the mount
 	theMount, found := mounts.FindMount(mountpoint)
 	if !found {
-		return errors.Errorf("%s is not a mountpoint", mountpoint)
-	}
-
-	err = unix.Unmount(mountpoint, 0)
-	if err != nil {
-		return errors.Wrapf(err, "failed unmounting %v", mountpoint)
+		return "", errors.Errorf("%s is not a mountpoint", mountpoint)
 	}
+	return theMount.Source, nil
+}
 
-	if _, err := os.Stat(theMount.Source); err != nil {
+// given a device path that had been used as the backing device for a squash
+// mountpoint, cleans up and detaches verity device if it still exists.
+//
+// If the device path does not exist, that is OK - this happens if the device
+// was a regular loopback and not -verity.
+func MaybeCleanupBackingDevice(devPath string) error {
+	if _, err := os.Stat(devPath); err != nil {
 		if os.IsNotExist(err) {
+			// It's been detached, this is fine.
 			return nil
 		}
 		return errors.WithStack(err)
@@ -476,9 +493,9 @@
 	// was this a verity mount or a regular loopback mount? (if it's a
 	// regular loopback mount, we detached it above, so need to do anything
 	// special here; verity doesn't play as nicely)
-	if strings.HasSuffix(theMount.Source, veritySuffix) {
+	if strings.HasSuffix(devPath, veritySuffix) {
 		// find the loop device that backs the verity device
-		deviceNo, err := findLoopBackingVerity(theMount.Source)
+		deviceNo, err := findLoopBackingVerity(devPath)
 		if err != nil {
 			return err
 		}
@@ -488,18 +505,22 @@
 		// above). the cryptsetup API allows us to pass NULL for the crypt
 		// device, but go-cryptsetup doesn't have a way to initialize a NULL
 		// crypt device short of making the struct by hand like this.
-		err = (&cryptsetup.Device{}).Deactivate(theMount.Source)
+		err = (&cryptsetup.Device{}).Deactivate(devPath)
 		if err != nil {
 			return errors.WithStack(err)
 		}
 
 		// finally, kill the loop dev
 		err = loopDev.Detach()
 		if err != nil {
-			return errors.Wrapf(err, "failed to detach loop dev for %v", theMount.Source)
+			return errors.Wrapf(err, "failed to detach loop dev for %v", devPath)
 		}
 	}
 
+	// NOTE: because of lazy device destruction, it is possible for a non-verity
+	// backing loop dev to be auto-detached but still exist. There's nothing for
+	// us to do in that case, so we return nil.
+
 	return nil
 }