From c11c5e180a6e00e0093f51b050962ee1e2e30f7a Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Mon, 30 Mar 2020 19:59:47 -0700 Subject: pkg/spec/initFSMounts: fix > $ ./bin/podman run -v /tmp:/tmp alpine true; echo $? > 0 > $ ./bin/podman run -v /tmp:/tmp:ro alpine true; echo $? > 0 > $ ./bin/podman run -v /tmp:/w0w:ro alpine true; echo $? > Error: container_linux.go:349: starting container process caused "process_linux.go:449: container init caused \"rootfs_linux.go:58: mounting \\\"/tmp\\\" to rootfs \\\"/home/kir/.local/share/containers/storage/overlay/7636ef3650fc91ee4996ccc026532bb3cff7182c0430db662fffb933e0bcadc9/merged\\\" at \\\"/home/kir/.local/share/containers/storage/overlay/7636ef3650fc91ee4996ccc026532bb3cff7182c0430db662fffb933e0bcadc9/merged/w0w\\\" caused \\\"operation not permitted\\\"\"": OCI runtime permission denied error > 126 The last command is not working because in-container mount point is used to search for a parent mount in /proc/self/mountinfo. And yet the following > $ ./bin/podman run -v /tmp:/run/test:ro alpine true; echo $? > 0 still works fine! Here's why: > $ mount | grep -E '/run |/tmp ' > tmpfs on /run type tmpfs (rw,nosuid,nodev,seclabel,mode=755) > tmpfs on /tmp type tmpfs (rw,nosuid,nodev,seclabel) This is the reason why previous commit modified in-container mount point. Fixes: 0f5ae3c5af Signed-off-by: Kir Kolyshkin --- pkg/spec/storage.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index b0687b4c2..404d94432 100644 --- a/pkg/spec/storage.go +++ b/pkg/spec/storage.go @@ -867,9 +867,9 @@ func InitFSMounts(inputMounts []spec.Mount) ([]spec.Mount, error) { var mounts []spec.Mount for _, m := range inputMounts { if m.Type == TypeBind { - baseMnt, err := findMount(m.Destination, systemMounts) + baseMnt, err := findMount(m.Source, systemMounts) if err != nil { - return nil, errors.Wrapf(err, "error looking up mountpoint for mount %s", m.Destination) + return nil, errors.Wrapf(err, "error looking up mountpoint for mount %s", m.Source) } var noexec, nosuid, nodev bool for _, baseOpt := range strings.Split(baseMnt.Opts, ",") { -- cgit v1.2.3-54-g00ecf From f2c42a3958d12b45375aeb2384a3a8a103203c1c Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 31 Mar 2020 10:20:05 -0700 Subject: pkg/spec.InitFSMounts: fix mount opts in place ... rather than create a new slice and then make the caller replace the original with the new one. Signed-off-by: Kir Kolyshkin --- pkg/spec/spec.go | 4 +--- pkg/spec/storage.go | 22 +++++++++------------- pkg/specgen/oci.go | 4 +--- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/pkg/spec/spec.go b/pkg/spec/spec.go index d4fd5976f..194d2fcb3 100644 --- a/pkg/spec/spec.go +++ b/pkg/spec/spec.go @@ -371,11 +371,9 @@ func (config *CreateConfig) createConfigToOCISpec(runtime *libpod.Runtime, userM // BIND MOUNTS configSpec.Mounts = SupercedeUserMounts(userMounts, configSpec.Mounts) // Process mounts to ensure correct options - finalMounts, err := InitFSMounts(configSpec.Mounts) - if err != nil { + if err := InitFSMounts(configSpec.Mounts); err != nil { return nil, err } - configSpec.Mounts = finalMounts // BLOCK IO blkio, err := config.CreateBlockIO() diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index 404d94432..335907d12 100644 --- a/pkg/spec/storage.go +++ b/pkg/spec/storage.go @@ -855,21 +855,19 @@ func SupercedeUserMounts(mounts []spec.Mount, configMount []spec.Mount) []spec.M } // Ensure mount options on all mounts are correct -func InitFSMounts(inputMounts []spec.Mount) ([]spec.Mount, error) { +func InitFSMounts(mounts []spec.Mount) error { // We need to look up mounts so we can figure out the proper mount flags // to apply. systemMounts, err := pmount.GetMounts() if err != nil { - return nil, errors.Wrapf(err, "error retrieving system mounts to look up mount options") + return errors.Wrapf(err, "error retrieving system mounts to look up mount options") } - // TODO: We probably don't need to re-build the mounts array - var mounts []spec.Mount - for _, m := range inputMounts { + for i, m := range mounts { if m.Type == TypeBind { baseMnt, err := findMount(m.Source, systemMounts) if err != nil { - return nil, errors.Wrapf(err, "error looking up mountpoint for mount %s", m.Source) + return errors.Wrapf(err, "error looking up mountpoint for mount %s", m.Source) } var noexec, nosuid, nodev bool for _, baseOpt := range strings.Split(baseMnt.Opts, ",") { @@ -890,21 +888,19 @@ func InitFSMounts(inputMounts []spec.Mount) ([]spec.Mount, error) { opts, err := util.ProcessOptions(m.Options, false, defaultMountOpts) if err != nil { - return nil, err + return err } - m.Options = opts + mounts[i].Options = opts } if m.Type == TypeTmpfs && filepath.Clean(m.Destination) != "/dev" { opts, err := util.ProcessOptions(m.Options, true, nil) if err != nil { - return nil, err + return err } - m.Options = opts + mounts[i].Options = opts } - - mounts = append(mounts, m) } - return mounts, nil + return nil } // TODO: We could make this a bit faster by building a tree of the mountpoints diff --git a/pkg/specgen/oci.go b/pkg/specgen/oci.go index 2523f21b3..db60dc25e 100644 --- a/pkg/specgen/oci.go +++ b/pkg/specgen/oci.go @@ -215,11 +215,9 @@ func (s *SpecGenerator) toOCISpec(rt *libpod.Runtime, newImage *image.Image) (*s // BIND MOUNTS configSpec.Mounts = createconfig.SupercedeUserMounts(s.Mounts, configSpec.Mounts) // Process mounts to ensure correct options - finalMounts, err := createconfig.InitFSMounts(configSpec.Mounts) - if err != nil { + if err := createconfig.InitFSMounts(configSpec.Mounts); err != nil { return nil, err } - configSpec.Mounts = finalMounts // Add annotations if configSpec.Annotations == nil { -- cgit v1.2.3-54-g00ecf From e0614367ca27c2e47649e08291e0f1d19d42232e Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Tue, 31 Mar 2020 01:59:44 -0700 Subject: pkg/spec.InitFSMounts: optimize Instead of getting mount options from /proc/self/mountinfo, which is very costly to read/parse (and can even be unreliable), let's use statfs(2) to figure out the flags we need. [v2: move getting default options to pkg/util, make it linux-specific] Signed-off-by: Kir Kolyshkin --- libpod/options.go | 2 +- pkg/spec/storage.go | 60 ++++----------------------------------------- pkg/util/mountOpts.go | 24 +++++++++--------- pkg/util/mountOpts_linux.go | 23 +++++++++++++++++ pkg/util/mountOpts_other.go | 7 ++++++ 5 files changed, 49 insertions(+), 67 deletions(-) create mode 100644 pkg/util/mountOpts_linux.go create mode 100644 pkg/util/mountOpts_other.go diff --git a/libpod/options.go b/libpod/options.go index dfbec364a..65a089131 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -1334,7 +1334,7 @@ func WithNamedVolumes(volumes []*ContainerNamedVolume) CtrCreateOption { } destinations[vol.Dest] = true - mountOpts, err := util.ProcessOptions(vol.Options, false, nil) + mountOpts, err := util.ProcessOptions(vol.Options, false, "") if err != nil { return errors.Wrapf(err, "error processing options for named volume %q mounted at %q", vol.Name, vol.Dest) } diff --git a/pkg/spec/storage.go b/pkg/spec/storage.go index 335907d12..68a84d638 100644 --- a/pkg/spec/storage.go +++ b/pkg/spec/storage.go @@ -10,7 +10,6 @@ import ( "github.com/containers/buildah/pkg/parse" "github.com/containers/libpod/libpod" "github.com/containers/libpod/pkg/util" - pmount "github.com/containers/storage/pkg/mount" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -856,44 +855,16 @@ func SupercedeUserMounts(mounts []spec.Mount, configMount []spec.Mount) []spec.M // Ensure mount options on all mounts are correct func InitFSMounts(mounts []spec.Mount) error { - // We need to look up mounts so we can figure out the proper mount flags - // to apply. - systemMounts, err := pmount.GetMounts() - if err != nil { - return errors.Wrapf(err, "error retrieving system mounts to look up mount options") - } - for i, m := range mounts { - if m.Type == TypeBind { - baseMnt, err := findMount(m.Source, systemMounts) - if err != nil { - return errors.Wrapf(err, "error looking up mountpoint for mount %s", m.Source) - } - var noexec, nosuid, nodev bool - for _, baseOpt := range strings.Split(baseMnt.Opts, ",") { - switch baseOpt { - case "noexec": - noexec = true - case "nosuid": - nosuid = true - case "nodev": - nodev = true - } - } - - defaultMountOpts := new(util.DefaultMountOptions) - defaultMountOpts.Noexec = noexec - defaultMountOpts.Nosuid = nosuid - defaultMountOpts.Nodev = nodev - - opts, err := util.ProcessOptions(m.Options, false, defaultMountOpts) + switch { + case m.Type == TypeBind: + opts, err := util.ProcessOptions(m.Options, false, m.Source) if err != nil { return err } mounts[i].Options = opts - } - if m.Type == TypeTmpfs && filepath.Clean(m.Destination) != "/dev" { - opts, err := util.ProcessOptions(m.Options, true, nil) + case m.Type == TypeTmpfs && filepath.Clean(m.Destination) != "/dev": + opts, err := util.ProcessOptions(m.Options, true, "") if err != nil { return err } @@ -902,24 +873,3 @@ func InitFSMounts(mounts []spec.Mount) error { } return nil } - -// TODO: We could make this a bit faster by building a tree of the mountpoints -// and traversing it to identify the correct mount. -func findMount(target string, mounts []*pmount.Info) (*pmount.Info, error) { - var err error - target, err = filepath.Abs(target) - if err != nil { - return nil, errors.Wrapf(err, "cannot resolve %s", target) - } - var bestSoFar *pmount.Info - for _, i := range mounts { - if bestSoFar != nil && len(bestSoFar.Mountpoint) > len(i.Mountpoint) { - // Won't be better than what we have already found - continue - } - if strings.HasPrefix(target, i.Mountpoint) { - bestSoFar = i - } - } - return bestSoFar, nil -} diff --git a/pkg/util/mountOpts.go b/pkg/util/mountOpts.go index d21800bc3..329a7c913 100644 --- a/pkg/util/mountOpts.go +++ b/pkg/util/mountOpts.go @@ -13,19 +13,17 @@ var ( ErrDupeMntOption = errors.Errorf("duplicate mount option passed") ) -// DefaultMountOptions sets default mount options for ProcessOptions. -type DefaultMountOptions struct { - Noexec bool - Nosuid bool - Nodev bool +type defaultMountOptions struct { + noexec bool + nosuid bool + nodev bool } // ProcessOptions parses the options for a bind or tmpfs mount and ensures that // they are sensible and follow convention. The isTmpfs variable controls // whether extra, tmpfs-specific options will be allowed. -// The defaults variable controls default mount options that will be set. If it -// is not included, they will be set unconditionally. -func ProcessOptions(options []string, isTmpfs bool, defaults *DefaultMountOptions) ([]string, error) { +// The sourcePath variable, if not empty, contains a bind mount source. +func ProcessOptions(options []string, isTmpfs bool, sourcePath string) ([]string, error) { var ( foundWrite, foundSize, foundProp, foundMode, foundExec, foundSuid, foundDev, foundCopyUp, foundBind, foundZ bool ) @@ -122,13 +120,17 @@ func ProcessOptions(options []string, isTmpfs bool, defaults *DefaultMountOption if !foundProp { newOptions = append(newOptions, "rprivate") } - if !foundExec && (defaults == nil || defaults.Noexec) { + defaults, err := getDefaultMountOptions(sourcePath) + if err != nil { + return nil, err + } + if !foundExec && defaults.noexec { newOptions = append(newOptions, "noexec") } - if !foundSuid && (defaults == nil || defaults.Nosuid) { + if !foundSuid && defaults.nosuid { newOptions = append(newOptions, "nosuid") } - if !foundDev && (defaults == nil || defaults.Nodev) { + if !foundDev && defaults.nodev { newOptions = append(newOptions, "nodev") } if isTmpfs && !foundCopyUp { diff --git a/pkg/util/mountOpts_linux.go b/pkg/util/mountOpts_linux.go new file mode 100644 index 000000000..3eac4dd25 --- /dev/null +++ b/pkg/util/mountOpts_linux.go @@ -0,0 +1,23 @@ +package util + +import ( + "os" + + "golang.org/x/sys/unix" +) + +func getDefaultMountOptions(path string) (defaultMountOptions, error) { + opts := defaultMountOptions{true, true, true} + if path == "" { + return opts, nil + } + var statfs unix.Statfs_t + if e := unix.Statfs(path, &statfs); e != nil { + return opts, &os.PathError{Op: "statfs", Path: path, Err: e} + } + opts.nodev = (statfs.Flags&unix.MS_NODEV == unix.MS_NODEV) + opts.noexec = (statfs.Flags&unix.MS_NOEXEC == unix.MS_NOEXEC) + opts.nosuid = (statfs.Flags&unix.MS_NOSUID == unix.MS_NOSUID) + + return opts, nil +} diff --git a/pkg/util/mountOpts_other.go b/pkg/util/mountOpts_other.go new file mode 100644 index 000000000..6a34942e5 --- /dev/null +++ b/pkg/util/mountOpts_other.go @@ -0,0 +1,7 @@ +// +build !linux + +package util + +func getDefaultMountOptions(path string) (opts defaultMountOptions, err error) { + return +} -- cgit v1.2.3-54-g00ecf