WIP- fix umount cleanup

Signed-off-by: Michael McCracken <[email protected]>

Makefile

@@ -46,10 +46,17 @@ $(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
+batstest: $(BATS) $(STACKER) atomfs test/random.txt testimages
+	cd $(ROOT)/test; sudo $(BATS) --tap --timing priv-mount-umount-mount.bats priv-mount.bats
 	cd $(ROOT)/test; $(BATS) --tap --timing unpriv-*.bats
 
+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.Umount(mountpoint, ctx.String("metadir"))
 }

molecule.go

@@ -287,7 +287,9 @@ func (m Molecule) Mount(dest string) error {
 	return nil
 }
 
-func Umount(dest string) error {
+func Umount(dest, metadir string) error {
+	log.Debugf("------- starting Umount(%q, %q)", dest, metadir)
+
 	var err error
 	dest, err = filepath.Abs(dest)
 	if err != nil {
@@ -310,13 +312,14 @@ func Umount(dest string) error {
 		return err
 	}
 
+	// Find all mountpoints underlying the current top Overlay MP
 	underlyingAtoms := []string{}
 	for _, m := range mounts {
 		if m.FSType != "overlay" {
 			continue
 		}
 
-		if m.Target != dest { // TODO is this still right
+		if m.Target != dest {
 			continue
 		}
 
@@ -331,56 +334,62 @@ func Umount(dest string) error {
 		return errors.Errorf("%s is not an atomfs mountpoint", dest)
 	}
 
+	// Unmount the top Overlay MP
+	log.Debugf("Unmounting dest=%q", dest)
 	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

oci.go

@@ -37,6 +37,20 @@ func (c MountOCIOpts) WriteToFile(filename string) error {
 	return nil
 }
 
+// func ReadOptsFromFile(filename string) (MountOCIOpts, error) {
+// 	var opts MountOCIOpts
+// 	data, err := os.ReadFile(filename)
+// 	if err != nil {
+// 		return opts, error
+// 	}
+
+// 	err := json.Unmarshal(data, &opts)
+// 	if err != nil {
+// 		return opts, err
+// 	}
+// 	return opts, nil
+// }
+
 func BuildMoleculeFromOCI(opts MountOCIOpts) (Molecule, error) {
 	oci, err := umoci.OpenLayout(opts.OCIDir)
 	if err != nil {

squashfs/verity.go

@@ -449,25 +449,42 @@ func findLoopBackingVerity(device string) (int64, error) {
 	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 @@ func Umount(mountpoint string) error {
 	// 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 @@ func Umount(mountpoint string) error {
 		// 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
 }
 

test/helpers.bash

@@ -12,7 +12,8 @@ export PATH="$TOOLS_D/bin:$ROOT_D/bin:$PATH"
 
 build_image_at() {
     cd $1
-    sudo env "PATH=$PATH" stacker --oci-dir $1/oci --stacker-dir=$1/stacker --roots-dir=$1/roots --debug build -f $(dirname $BATS_TEST_FILENAME)/1.stacker.yaml --layer-type squashfs
-    sudo env "PATH=$PATH" stacker --oci-dir $1/oci-no-verity --stacker-dir=$1/stacker --roots-dir=$1/roots  --debug build -f $(dirname $BATS_TEST_FILENAME)/1.stacker.yaml --layer-type squashfs --no-squashfs-verity
+    stackerfilename=${2:-1.stacker.yaml}
+    sudo env "PATH=$PATH" stacker --oci-dir $1/oci --stacker-dir=$1/stacker --roots-dir=$1/roots --debug build -f $(dirname $BATS_TEST_FILENAME)/$stackerfilename --layer-type squashfs
+    sudo env "PATH=$PATH" stacker --oci-dir $1/oci-no-verity --stacker-dir=$1/stacker --roots-dir=$1/roots  --debug build -f $(dirname $BATS_TEST_FILENAME)/$stackerfilename --layer-type squashfs --no-squashfs-verity
     sudo chown -R $(id -un):$(id -gn) $1/oci $1/oci-no-verity $1/stacker $1/roots
 }

test/mount-umount-trees.stacker.yaml

@@ -0,0 +1,20 @@
+base:
+    from:
+        type: oci
+        url: /tmp/atomfs-test-oci:busybox
+    run: touch /base
+a:
+    from:
+        type: built
+        tag: base
+    run: touch /a
+b:
+    from:
+        type: built
+        tag: base
+    run: touch /b
+c:
+    from:
+        type: built
+        tag: base
+    run: touch /c