From b467bdd0da563b4938e6ee17171150cf1b94cfec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alfonso=20S=C3=A1nchez-Beato?= Date: Thu, 5 Jan 2023 11:20:11 +0000 Subject: [PATCH] cmd/snap-update-ns: prevent keeping unneeded mountpoints In some cases we were keeping mounts from removed layouts, because we were not identifying properly the mounts that we wanted to keep, as we used mountpoint as key for the reuse map used in neededChanges, while there are corner cases when we mount more that once on a given mountpoint. This can happen when, say, we have a layout for /dir/sd1 and another one for /dir/sd2/sd3, being the case that /dir/sd1 and /dir/sd2/sd3 do not exist (but their parent dirs do exist) - /dir/sd2 will be one of the bind mounted directories of the tmpfs that is created in /dir to have a layout on /dir/sd1, while at the same time a tmpfs will be mounted in /dir/sd2 so we can have a layout in /dir/sd2/sd3. So /dir/sd2 is used twice with different filesystem types (none and tmpfs). To prevent this, use as key in the reuse map dir+fstype. As we make sure that mimics are created only once per directory, we should only have one entry per dir+fstype, being fstype either none or tmpfs. In the future we should clean-up mountpoints before performing changes, but that is a bigger change and anyway this fix is needed for mount namespaces already created by older snapd versions. --- cmd/snap-update-ns/change.go | 36 +++++++++++++++++++++++++------ cmd/snap-update-ns/change_test.go | 26 ++++++++++++++++++++++ 2 files changed, 56 insertions(+), 6 deletions(-) diff --git a/cmd/snap-update-ns/change.go b/cmd/snap-update-ns/change.go index e88509bb649..6772388c8ca 100644 --- a/cmd/snap-update-ns/change.go +++ b/cmd/snap-update-ns/change.go @@ -493,6 +493,29 @@ func (c *Change) lowLevelPerform(as *Assumptions) error { return fmt.Errorf("cannot process mount change: unknown action: %q", c.Action) } +// Using dir is not enough to identify the mount entry, because when +// using layouts some directories could be used as mount points more +// than once. This can happen when, say, we have a layout for /dir/sd1 +// and another one for /dir/sd2/sd3, being the case that /dir/sd1 and +// /dir/sd2/sd3 do not exist (but their parent dirs do exist) - +// /dir/sd2 will be one of the bind mounted directories of the tmpfs +// that is created in /dir to have a layout on /dir/sd1, while at the +// same time a tmpfs will be mounted in /dir/sd2 so we can have a +// layout in /dir/sd2/sd3. So /dir/sd2 is used twice with different +// filesystem types (none and tmpfs). As we make sure that mimics are +// created only once per directory, we should only have one entry per +// dir+fstype, being fstype either none or tmpfs. +// +// TODO Ideally we should have only one mount per mountpoint, but we +// perform mounts as we create the changes in Change.Perform which +// makes it difficult to create the full list of changes and then +// clean-up repeated mountpoints. In any case using this is still +// needed to handle mount namespaces created by older snapd versions. +type mountEntryId struct { + dir string + fsType string +} + // neededChanges is the real implementation of NeededChanges func neededChanges(currentProfile, desiredProfile *osutil.MountProfile) []*Change { // Copy both profiles as we will want to mutate them. @@ -539,7 +562,7 @@ func neededChanges(currentProfile, desiredProfile *osutil.MountProfile) []*Chang } // Indexed by mount point path. - reuse := make(map[string]bool) + reuse := make(map[mountEntryId]bool) // Indexed by entry ID desiredIDs := make(map[string]bool) var skipDir string @@ -562,10 +585,11 @@ func neededChanges(currentProfile, desiredProfile *osutil.MountProfile) []*Chang } skipDir = "" // reset skip prefix as it no longer applies + mountId := mountEntryId{dir, current[i].Type} if current[i].XSnapdOrigin() == "rootfs" { // This is the rootfs setup by snap-confine, we should not touch it logger.Debugf("reusing rootfs") - reuse[dir] = true + reuse[mountId] = true continue } @@ -587,14 +611,14 @@ func neededChanges(currentProfile, desiredProfile *osutil.MountProfile) []*Chang // fact was lost. if current[i].XSnapdSynthetic() && desiredIDs[current[i].XSnapdNeededBy()] { logger.Debugf("reusing synthetic entry %q", current[i]) - reuse[dir] = true + reuse[mountId] = true continue } // Reuse entries that are desired and identical in the current profile. if entry, ok := desiredMap[dir]; ok && current[i].Equal(entry) { logger.Debugf("reusing unchanged entry %q", current[i]) - reuse[dir] = true + reuse[mountId] = true continue } @@ -610,7 +634,7 @@ func neededChanges(currentProfile, desiredProfile *osutil.MountProfile) []*Chang // Unmount entries not reused in reverse to handle children before their parent. unmountOrder := unsortedCurrent for i := len(unmountOrder) - 1; i >= 0; i-- { - if reuse[unmountOrder[i].Dir] { + if reuse[mountEntryId{unmountOrder[i].Dir, unmountOrder[i].Type}] { changes = append(changes, &Change{Action: Keep, Entry: unmountOrder[i]}) } else { var entry osutil.MountEntry = unmountOrder[i] @@ -627,7 +651,7 @@ func neededChanges(currentProfile, desiredProfile *osutil.MountProfile) []*Chang var desiredNotReused []osutil.MountEntry for _, entry := range desired { - if !reuse[entry.Dir] { + if !reuse[mountEntryId{entry.Dir, entry.Type}] { desiredNotReused = append(desiredNotReused, entry) } } diff --git a/cmd/snap-update-ns/change_test.go b/cmd/snap-update-ns/change_test.go index 43505765cbe..918982f042a 100644 --- a/cmd/snap-update-ns/change_test.go +++ b/cmd/snap-update-ns/change_test.go @@ -518,6 +518,32 @@ func (s *changeSuite) TestNeededChangesParallelInstancesInsideMount(c *C) { }) } +func (s *changeSuite) TestNeededChangesRepeatedDir(c *C) { + desired := &osutil.MountProfile{Entries: []osutil.MountEntry{ + {Name: "tmpfs", Dir: "/foo/mytmp", Type: "tmpfs", Options: []string{osutil.XSnapdOriginLayout()}}, + }} + current := &osutil.MountProfile{Entries: []osutil.MountEntry{ + {Name: "/foo/bar", Dir: "/foo/bar", Type: "none", + Options: []string{osutil.XSnapdSynthetic(), osutil.XSnapdNeededBy("/foo/mytmp")}}, + {Name: "tmpfs", Dir: "/foo/mytmp", Type: "tmpfs", Options: []string{osutil.XSnapdOriginLayout()}}, + {Name: "tmpfs", Dir: "/foo/bar", Type: "tmpfs", + Options: []string{osutil.XSnapdSynthetic(), osutil.XSnapdNeededBy("/foo/bar/two")}}, + }} + changes := update.NeededChanges(current, desired) + + // Make sure that we unmount the one that is needed by an entry + // not desired anymore (even though it is in the same mountpoint + // as the one needed by /foo/mytmp). + c.Assert(changes, DeepEquals, []*update.Change{ + {Entry: osutil.MountEntry{Name: "tmpfs", Dir: "/foo/bar", Type: "tmpfs", + Options: []string{osutil.XSnapdSynthetic(), osutil.XSnapdNeededBy("/foo/bar/two"), osutil.XSnapdDetach()}}, Action: update.Unmount}, + {Entry: osutil.MountEntry{Name: "tmpfs", Dir: "/foo/mytmp", Type: "tmpfs", + Options: []string{osutil.XSnapdOriginLayout()}}, Action: update.Keep}, + {Entry: osutil.MountEntry{Name: "/foo/bar", Dir: "/foo/bar", Type: "none", + Options: []string{osutil.XSnapdSynthetic(), osutil.XSnapdNeededBy("/foo/mytmp")}}, Action: update.Keep}, + }) +} + func (s *changeSuite) TestRuntimeUsingSymlinks(c *C) { dirs.SetRootDir(c.MkDir()) defer func() {