From d19c3bc9bcdf36fc592d634e9337dd4f84bec02d Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Fri, 16 May 2025 18:18:58 -0700 Subject: [PATCH] runsc: create file as destination for file mount Linux does not allow mounting files on top of directories and vice versa. This can lead to unexpected behavior and issues, such as `dockerd` failing to start when it encounters a file mount that appears as a directory entry during `/dev` enumeration. After this change, new mount destinations (files or directories) are created with permissions (0644 for files, 0755 for directories) consistent with `runc` behavior. Previously, new directories were created with 0777. The test case that request a bind mount into /dev/fd has been removed. /dev/fd is a symlink to /proc/self/fd. PiperOrigin-RevId: 759827816 --- runsc/boot/BUILD | 1 + runsc/boot/loader_test.go | 118 ++++++++++++++++++++++++++++++++++-- runsc/boot/vfs.go | 123 ++++++++++++++++++++++++++++++++------ 3 files changed, 219 insertions(+), 23 deletions(-) diff --git a/runsc/boot/BUILD b/runsc/boot/BUILD index 01f5c8b687..ebd6bd8734 100644 --- a/runsc/boot/BUILD +++ b/runsc/boot/BUILD @@ -156,6 +156,7 @@ go_test( ], library = ":boot", deps = [ + "//pkg/abi/linux", "//pkg/control/server", "//pkg/cpuid", "//pkg/fspath", diff --git a/runsc/boot/loader_test.go b/runsc/boot/loader_test.go index 6b3a1da384..51baeeffe7 100644 --- a/runsc/boot/loader_test.go +++ b/runsc/boot/loader_test.go @@ -25,6 +25,7 @@ import ( "github.com/moby/sys/capability" specs "github.com/opencontainers/runtime-spec/specs-go" "golang.org/x/sys/unix" + "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/control/server" "gvisor.dev/gvisor/pkg/cpuid" "gvisor.dev/gvisor/pkg/fspath" @@ -403,11 +404,6 @@ func createMountTestcases() []*CreateMountTestcase { Destination: "/dev", Type: "tmpfs", }, - { - // Mounted by runsc by default. - Destination: "/dev/fd", - Type: "tmpfs", - }, { // Mount with the same prefix. Destination: "/dev/fd-foo", @@ -508,6 +504,118 @@ func TestCreateMountNamespace(t *testing.T) { } } +type createMountPointTestCase struct { + name string + path string + mode linux.FileMode + fail bool +} + +func TestCreateMountPoint(t *testing.T) { + var createMountPointTestCases = []createMountPointTestCase{ + { + name: "File", + path: "/test/test-file", + mode: linux.ModeRegular, + }, + { + name: "Directory", + path: "/test/test-dir", + mode: linux.ModeDirectory, + }, + { + name: "FileWithIntDirs", + path: "/test/a/b/c/test-file", + mode: linux.ModeRegular, + }, + { + name: "DirectoryWithIntDirs", + path: "/test/d/e/f/g/test-dir", + mode: linux.ModeDirectory, + }, + { + name: "ExistingFile", + path: "/test/test-file", + mode: linux.ModeRegular, + }, + { + name: "ExistingDirectory", + path: "/test/test-dir", + mode: linux.ModeDirectory, + }, + { + name: "DirVSFile", + path: "/test/test-file", + mode: linux.ModeDirectory, + fail: true, + }, + { + name: "FileVSDir", + path: "/test/test-dir", + mode: linux.ModeRegular, + fail: true, + }, + } + + spec := testSpec() + spec.Root = &specs.Root{ + Path: os.TempDir(), + } + spec.Mounts = append(spec.Mounts, specs.Mount{ + Destination: "/test", + Type: "tmpfs", + }) + + t.Logf("Using root: %q", spec.Root.Path) + l, loaderCleanup, err := createLoader(testConfig(), spec) + if err != nil { + t.Fatalf("failed to create loader: %v", err) + } + defer l.Destroy() + defer loaderCleanup() + + l.mu.Lock() + defer l.mu.Unlock() + mntr := newContainerMounter(&l.root, l.k, l.mountHints, l.sharedMounts, "", l.sandboxID) + ctx := l.k.SupervisorContext() + creds := auth.NewRootCredentials(l.root.procArgs.Credentials.UserNamespace) + mns, err := mntr.mountAll(ctx, creds, l.root.spec, l.root.conf, &l.root.procArgs) + if err != nil { + t.Fatalf("mountAll: %v", err) + } + + root := mns.Root(ctx) + defer root.DecRef(ctx) + + for _, tc := range createMountPointTestCases { + t.Run(tc.name, func(t *testing.T) { + if err := mntr.makeMountPoint(ctx, creds, mns, tc.path, tc.mode); err != nil { + if tc.fail { + return + } + t.Fatalf("makeMountPoint failed: %v", err) + } else { + if tc.fail { + t.Fatalf("makeMountPoint doesn't fail") + } + } + target := &vfs.PathOperation{ + Root: root, + Start: root, + Path: fspath.Parse(tc.path), + } + + if mode, err := mntr.getPathMode(ctx, creds, target); err != nil { + t.Errorf("expected path %v, but got error %v", tc.path, err) + } else { + if mode.IsDir() != tc.mode.IsDir() { + t.Errorf("path mode %v doesn't match the mount mode %v", mode, tc.mode) + } + } + }) + } +} + func TestMain(m *testing.M) { cpuid.Initialize() seccheck.Initialize() diff --git a/runsc/boot/vfs.go b/runsc/boot/vfs.go index 69b7879bf9..cf7aa06bee 100644 --- a/runsc/boot/vfs.go +++ b/runsc/boot/vfs.go @@ -813,6 +813,20 @@ func (c *containerMounter) prepareMounts() ([]mountInfo, error) { return mounts, nil } +// getPathMode returns file mode for the specified path. +func (c *containerMounter) getPathMode(ctx context.Context, creds *auth.Credentials, path *vfs.PathOperation) (linux.FileMode, error) { + stat, err := c.k.VFS().StatAt(ctx, creds, path, &vfs.StatOptions{ + Mask: linux.STATX_TYPE, + }) + if err != nil { + return 0, err + } + if stat.Mask&linux.STATX_TYPE == 0 { + return 0, fmt.Errorf("failed to get file type") + } + return linux.FileMode(stat.Mode), nil +} + func (c *containerMounter) mountSubmount(ctx context.Context, spec *specs.Spec, conf *config.Config, mns *vfs.MountNamespace, creds *auth.Credentials, submount *mountInfo) (*vfs.Mount, error) { fsName, opts, err := getMountNameAndOptions(spec, conf, submount, c.productName, c.containerName) if err != nil { @@ -823,10 +837,6 @@ func (c *containerMounter) mountSubmount(ctx context.Context, spec *specs.Spec, return nil, nil } - if err := c.makeMountPoint(ctx, creds, mns, submount.mount.Destination); err != nil { - return nil, fmt.Errorf("creating mount point %q: %w", submount.mount.Destination, err) - } - if submount.goferMountConf.ShouldUseOverlayfs() { log.Infof("Adding overlay on top of mount %q", submount.mount.Destination) var cleanup func() @@ -838,18 +848,42 @@ func (c *containerMounter) mountSubmount(ctx context.Context, spec *specs.Spec, fsName = overlay.Name } + mount := submount.mount root := mns.Root(ctx) defer root.DecRef(ctx) target := &vfs.PathOperation{ Root: root, Start: root, - Path: fspath.Parse(submount.mount.Destination), + Path: fspath.Parse(mount.Destination), + } + mnt, err := c.k.VFS().MountDisconnected(ctx, creds, "", fsName, opts) + if err != nil { + return nil, fmt.Errorf("mounting %q (type:%s, dest: %q): %w, opts: %v", + mount.Source, mount.Type, mount.Destination, err, opts) } - mnt, err := c.k.VFS().MountAt(ctx, creds, "", target, fsName, opts) + defer mnt.DecRef(ctx) + + vd := vfs.MakeVirtualDentry(mnt, mnt.Root()) + rootPath := &vfs.PathOperation{ + Root: vd, + Start: vd, + } + + rootMode, err := c.getPathMode(ctx, creds, rootPath) if err != nil { - return nil, fmt.Errorf("failed to mount %q (type: %s): %w, opts: %v", submount.mount.Destination, submount.mount.Type, err, opts) + return nil, fmt.Errorf("stat mount %q (dest: %q): %w", mount.Source, mount.Destination, err) + } + if err := c.makeMountPoint(ctx, creds, mns, mount.Destination, rootMode); err != nil { + return nil, fmt.Errorf("creating mount point %q: %w", mount.Destination, err) } - log.Infof("Mounted %q to %q type: %s, internal-options: %q", submount.mount.Source, submount.mount.Destination, submount.mount.Type, opts.GetFilesystemOptions.Data) + + if err := c.k.VFS().ConnectMountAt(ctx, creds, mnt, target); err != nil { + return nil, fmt.Errorf("attaching %q to %q (type: %s): %w, opts: %v", + mount.Source, mount.Destination, mount.Type, err, opts) + } + + log.Infof("Mounted %q to %q type: %s, internal-options: %q", + mount.Source, mount.Destination, mount.Type, opts.GetFilesystemOptions.Data) return mnt, nil } @@ -1225,7 +1259,17 @@ func (c *containerMounter) mountSharedSubmount(ctx context.Context, conf *config Path: fspath.Parse(mntInfo.mount.Destination), } - if err := c.makeMountPoint(ctx, creds, mns, mntInfo.mount.Destination); err != nil { + vd := vfs.MakeVirtualDentry(newMnt, newMnt.Root()) + rootPath := &vfs.PathOperation{ + Root: vd, + Start: vd, + } + rootMode, err := c.getPathMode(ctx, creds, rootPath) + if err != nil { + return nil, err + } + + if err := c.makeMountPoint(ctx, creds, mns, mntInfo.mount.Destination, rootMode); err != nil { return nil, fmt.Errorf("creating mount point %q: %w", mntInfo.mount.Destination, err) } @@ -1236,24 +1280,67 @@ func (c *containerMounter) mountSharedSubmount(ctx context.Context, conf *config return newMnt, nil } -func (c *containerMounter) makeMountPoint(ctx context.Context, creds *auth.Credentials, mns *vfs.MountNamespace, dest string) error { +// makeMountPoint creates the mount point destination, ensuring its type (file +// or directory) matches `rootMode`. If the destination already exists, it just +// verifies that its type is consistent with `rootMode`; otherwise, the ENOTDIR +// error is returned. +func (c *containerMounter) makeMountPoint( + ctx context.Context, + creds *auth.Credentials, + mns *vfs.MountNamespace, + dest string, + rootMode linux.FileMode, +) error { root := mns.Root(ctx) defer root.DecRef(ctx) + target := &vfs.PathOperation{ Root: root, Start: root, Path: fspath.Parse(dest), } - // First check if mount point exists. When overlay is enabled, gofer doesn't - // allow changes to the FS, making MakeSytheticMountpoint() ineffective - // because MkdirAt fails with EROFS even if file exists. - vd, err := c.k.VFS().GetDentryAt(ctx, creds, target, &vfs.GetDentryOptions{}) - if err == nil { - // File exists, we're done. - vd.DecRef(ctx) + + fs := c.k.VFS() + + mode, err := c.getPathMode(ctx, creds, target) + // First check if mount point exists. + switch { + case err == nil: + if mode.IsDir() != rootMode.IsDir() { + if rootMode.IsDir() { + return fmt.Errorf("mountpoint %q isn't a directory", dest) + } else { + return fmt.Errorf("mountpoint %q isn't not a file", dest) + } + } + // Target already exists. return nil + case linuxerr.Equals(linuxerr.ENOENT, err): + // Expected, we will create the mount point. + default: + return fmt.Errorf("stat failed for %q during mountpoint creation: %w", dest, err) + } + + mkdirOpts := &vfs.MkdirOptions{Mode: 0755, ForSyntheticMountpoint: true} + + // Make sure the parent directory of target exists. + if err := fs.MkdirAllAt(ctx, path.Dir(dest), root, creds, mkdirOpts, true /* mustBeDir */); err != nil { + return fmt.Errorf("failed to create parent directory of mountpoint %q: %w", dest, err) + } + + if rootMode.IsDir() { + if err := fs.MkdirAt(ctx, creds, target, mkdirOpts); err != nil { + return fmt.Errorf("failed to create directory mountpoint %q: %w", dest, err) + } + } else { + mknodOpts := &vfs.MknodOptions{ + Mode: linux.FileMode(linux.S_IFREG | 0644), + } + if err := fs.MknodAt(ctx, creds, target, mknodOpts); err != nil { + return fmt.Errorf("failed to create file mountpoint %q: %w", dest, err) + } } - return c.k.VFS().MakeSyntheticMountpoint(ctx, dest, root, creds) + return nil } // configureRestore returns an updated context.Context including filesystem