Skip to content

Commit

Permalink
fix umount cleanup with new metadata location
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
mikemccracken committed Dec 19, 2024
1 parent eaa7b43 commit 8e93665
Show file tree
Hide file tree
Showing 8 changed files with 237 additions and 107 deletions.
9 changes: 8 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
54 changes: 1 addition & 53 deletions cmd/atomfs/umount.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,7 @@ package main

import (
"fmt"
"os"
"path/filepath"
"syscall"

"github.com/urfave/cli"
"machinerun.io/atomfs"
Expand Down Expand Up @@ -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)
Expand All @@ -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"))
}
110 changes: 72 additions & 38 deletions molecule.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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
Expand Down
47 changes: 34 additions & 13 deletions squashfs/verity.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
}
Expand All @@ -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
}

Expand Down
5 changes: 3 additions & 2 deletions test/helpers.bash
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
20 changes: 20 additions & 0 deletions test/mount-umount-trees.stacker.yaml
Original file line number Diff line number Diff line change
@@ -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
Loading

0 comments on commit 8e93665

Please sign in to comment.