From 8e9366550f29febb370b9fff621912b9027c1186 Mon Sep 17 00:00:00 2001 From: Michael McCracken Date: Mon, 16 Dec 2024 17:14:50 -0800 Subject: [PATCH] fix umount cleanup with new metadata location Since eaa7b43, Umount was not cleaning up backing devices correctly, but this was not caught in tests. Stacker's test suite had a test that noticed. It isn't clear that Umount as used in the `atomfs` cli tool was correct before eaa7b43 either, because the underlying atom mount points were put under the overmounted final mount point, and would thus not be shared by other molecule mounts. This also wasn't being tested though. This commit fixes Umount, and: - consolidates all unmounting logic - out of `cmd/umount` and into the package in `atomfs.Umount`. - adds a version of the test from Stacker that noticed this - that test belongs here instead of in stacker. Also fixed a couple seeming typos in that test that hid failures. - fixes the lock path used in Umount to match that used in Mount, which was out of sync since eaa7b43 - splits out the verity.go squashfs.Umount code to allow for getting backing devices of an atom without unmounting it, in order to only clean up the backing dev if it is no longer in use Signed-off-by: Michael McCracken --- Makefile | 9 ++- cmd/atomfs/umount.go | 54 +------------ molecule.go | 110 ++++++++++++++++++--------- squashfs/verity.go | 47 ++++++++---- test/helpers.bash | 5 +- test/mount-umount-trees.stacker.yaml | 20 +++++ test/priv-mount-umount-mount.bats | 97 +++++++++++++++++++++++ test/unpriv-guestmount.bats | 2 + 8 files changed, 237 insertions(+), 107 deletions(-) create mode 100644 test/mount-umount-trees.stacker.yaml create mode 100644 test/priv-mount-umount-mount.bats diff --git a/Makefile b/Makefile index a9df465..345a2a5 100644 --- a/Makefile +++ b/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 +batstest: $(BATS) $(STACKER) atomfs test/random.txt testimages cd $(ROOT)/test; sudo $(BATS) --tap --timing priv-*.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 diff --git a/cmd/atomfs/umount.go b/cmd/atomfs/umount.go index fde7340..2e123d7 100644 --- a/cmd/atomfs/umount.go +++ b/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")) } diff --git a/molecule.go b/molecule.go index f22e0c9..97d4db5 100644 --- a/molecule.go +++ b/molecule.go @@ -287,14 +287,32 @@ func (m Molecule) Mount(dest string) error { return nil } -func Umount(dest string) error { +// Default Umount passes "" and uses /run/atomfs metadir, see RuntimeDir(). +func Umount(dest) 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 @@ func Umount(dest string) error { 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 diff --git a/squashfs/verity.go b/squashfs/verity.go index ea1bf7b..c99fd68 100644 --- a/squashfs/verity.go +++ b/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,7 +505,7 @@ 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) } @@ -496,10 +513,14 @@ func Umount(mountpoint string) error { // 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 } diff --git a/test/helpers.bash b/test/helpers.bash index e8599c6..ef8fecd 100644 --- a/test/helpers.bash +++ b/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 } diff --git a/test/mount-umount-trees.stacker.yaml b/test/mount-umount-trees.stacker.yaml new file mode 100644 index 0000000..95de3fe --- /dev/null +++ b/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 diff --git a/test/priv-mount-umount-mount.bats b/test/priv-mount-umount-mount.bats new file mode 100644 index 0000000..9c24aaa --- /dev/null +++ b/test/priv-mount-umount-mount.bats @@ -0,0 +1,97 @@ +load helpers +load 'test_helper/bats-support/load' +load 'test_helper/bats-assert/load' +load 'test_helper/bats-file/load' + +function setup_file() { + check_root + build_image_at $BATS_SUITE_TMPDIR mount-umount-trees.stacker.yaml + export ATOMFS_TEST_RUN_DIR=${BATS_SUITE_TMPDIR}/run/atomfs + mkdir -p $ATOMFS_TEST_RUN_DIR + export MY_MNTNSNAME=$(readlink /proc/self/ns/mnt | cut -c 6-15) +} + +function setup() { + export MP=${BATS_TEST_TMPDIR}/testmountpoint + mkdir -p $MP +} + +function verity_checkusedloops() { + # search for loopdevices which have backing files with the current + # BATS_TEST_DIRNAME value and complain if they're present. + local usedloops="" found="" x="" + for ((x=0; x<5; x++)); do + usedloops=$(losetup -a | grep $BATS_TEST_DIRNAME || echo) + if [ -n "$usedloops" ]; then + found=1 + udevadm settle + else + return 0 + fi + done + echo "found used loops in testdir=$BATS_TEST_DIRNAME :$usedloops" >&3 + [ $found = 1 ] +} + +@test "mount + umount + mount a tree of images works" { + + echo MOUNT A + mkdir -p $MP/a + run atomfs --debug mount ${BATS_SUITE_TMPDIR}/oci:a-squashfs $MP/a + assert_success + assert_file_exists $MP/a/a + + echo MOUNT B + mkdir -p $MP/b + run atomfs --debug mount ${BATS_SUITE_TMPDIR}/oci:b-squashfs $MP/b + assert_success + assert_file_exists $MP/b/b + + echo UMOUNT B + atomfs --debug umount $MP/b + assert_success + + # first layer should still exist since a is still mounted + manifest=$(cat ${BATS_SUITE_TMPDIR}/oci/index.json | jq -r .manifests[0].digest | cut -f2 -d:) + first_layer_hash=$(cat ${BATS_SUITE_TMPDIR}/oci/blobs/sha256/$manifest | jq -r .layers[0].digest | cut -f2 -d:) + assert_block_exists "/dev/mapper/$first_layer_hash-verity" + + echo MOUNT C + mkdir -p $MP/c + atomfs --debug mount ${BATS_SUITE_TMPDIR}/oci:c-squashfs $MP/c + assert_success + assert_file_exists $MP/c/c + + echo UMOUNT A + atomfs --debug umount $MP/a + assert_success + + # first layer should still exist since c is still mounted + manifest=$(cat ${BATS_SUITE_TMPDIR}/oci/index.json | jq -r .manifests[0].digest | cut -f2 -d:) + first_layer_hash=$(cat ${BATS_SUITE_TMPDIR}/oci/blobs/sha256/$manifest | jq -r .layers[0].digest | cut -f2 -d:) + assert_block_exists "/dev/mapper/$first_layer_hash-verity" + + # c should still be ok + assert_file_exists $MP/c/c + assert_file_exists $MP/c/bin/sh + + atomfs --debug umount $MP/c + assert_success + + # c's last layer shouldn't exist any more, since it is unique + manifest=$(cat ${BATS_SUITE_TMPDIR}/oci/index.json | jq -r .manifests[0].digest | cut -f2 -d:) + last_layer_num=$(($(cat ${BATS_SUITE_TMPDIR}/oci/blobs/sha256/$manifest | jq -r '.layers | length')-1)) + last_layer_hash=$(cat ${BATS_SUITE_TMPDIR}/oci/blobs/sha256/$manifest | jq -r .layers[$last_layer_num].digest | cut -f2 -d:) + echo "last layer hash is $last_layer_hash" + assert_block_not_exists /dev/mapper/$last_layer_hash-verity + verity_checkusedloops + + # mount points and meta dir should exist but be empty: + for subdir in a b c; do + assert_dir_exists $MP/$subdir + assert [ -z $( ls -A $MP/$subdir) ] + done + assert_dir_exists $ATOMFS_TEST_RUN_DIR/meta/$MY_MNTNSNAME/ + assert [ -z $( ls -A $ATOMFS_TEST_RUN_DIR/meta/$MY_MNTNSNAME/) ] + +} diff --git a/test/unpriv-guestmount.bats b/test/unpriv-guestmount.bats index a2db887..515b3e1 100644 --- a/test/unpriv-guestmount.bats +++ b/test/unpriv-guestmount.bats @@ -31,6 +31,7 @@ function setup() { echo guestmount without allow-missing should fail, because we do not have verity exit 1 } + echo "XFAIL: guestmount without allow-missing did fail" set -e atomfs --debug mount --allow-missing-verity --persist=\$PERSIST_DIR ${BATS_SUITE_TMPDIR}/oci:test-squashfs $MP @@ -44,6 +45,7 @@ function setup() { echo mount with squashfuse ignores verity, so verify should have failed, output should include warning exit 1 } + echo "XFAIL: verify did fail on squashfuse mounted molecule" set -e find $ATOMFS_TEST_RUN_DIR/meta/\$INNER_MNTNSNAME/ -name config.json|xargs cat