diff --git a/Makefile b/Makefile index a9df465..a0b7ccd 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 - 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 diff --git a/cmd/atomfs/umount.go b/cmd/atomfs/umount.go index fde7340..ee91e55 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.Umount(mountpoint, ctx.String("metadir")) } diff --git a/molecule.go b/molecule.go index f22e0c9..ef32062 100644 --- a/molecule.go +++ b/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 diff --git a/oci.go b/oci.go index 500247e..b6470b9 100644 --- a/oci.go +++ b/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 { 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..ea942f7 --- /dev/null +++ b/test/priv-mount-umount-mount.bats @@ -0,0 +1,107 @@ +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 + + #cat /proc/self/mountinfo + #echo "mountinfo after 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 + + #cat /proc/self/mountinfo + #echo "mountinfo after c^" + + echo UMOUNT A + atomfs --debug umount $MP/a + assert_success + + + #cat /proc/self/mountinfo + #echo "mountinfo after umount a^" + + # 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..1bb715e 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,11 +45,14 @@ 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 find $ATOMFS_TEST_RUN_DIR/meta/\$INNER_MNTNSNAME/ + cat /proc/self/mountinfo + atomfs --debug umount $MP [ -f \$PERSIST_DIR/persist/let-me-write ]