diff --git a/client/client_mount_test.go b/client/client_mount_test.go index c5deb2300758..6dbbb5b34ca1 100644 --- a/client/client_mount_test.go +++ b/client/client_mount_test.go @@ -373,6 +373,91 @@ func testMountStubsDirectory(t *testing.T, sb integration.Sandbox) { }, keys) } +// testMountStubsRuntimeMountpoints verifies that an exec leaves behind the same +// directories for the runtime's own mount points regardless of the worker. The runtime +// creates /proc, /sys and /dev in the rootfs when the image does not ship them, and +// unlike the mount stubs of the exec's own mounts these are not cleaned up again. The +// rootless spec conversion drops the /sys mount, so without care a rootless worker +// produces a different image than a rootful one for the same build. +// https://github.com/moby/buildkit/issues/6686 +func testMountStubsRuntimeMountpoints(t *testing.T, sb integration.Sandbox) { + // Skipped on Windows because the runtime mount points and the stub cleanup behavior + // tied to them are Linux-specific. + integration.SkipOnPlatform(t, "windows", "Linux-specific runtime mount points") + c, err := New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + // busybox ships /dev but neither /proc nor /sys, so dropping /dev leaves a base + // without any of the runtime mount points. Building the base from the image rather + // than assembling one keeps this independent of the image layout, which differs + // between platforms. + base := llb.Image("busybox:latest").File(llb.Rm("/dev")) + + entries := func(t *testing.T, st llb.State) map[string]*testutil.TarItem { + t.Helper() + def, err := st.Marshal(sb.Context()) + require.NoError(t, err) + + tarFile := filepath.Join(t.TempDir(), "out.tar") + tarFileW, err := os.Create(tarFile) + require.NoError(t, err) + defer tarFileW.Close() + + _, err = c.Solve(sb.Context(), def, SolveOpt{ + Exports: []ExportEntry{ + { + Type: ExporterTar, + Output: fixedWriteCloser(tarFileW), + }, + }, + }, nil) + require.NoError(t, err) + tarFileW.Close() + + dt, err := os.ReadFile(tarFile) + require.NoError(t, err) + m, err := testutil.ReadTarToMap(dt, false) + require.NoError(t, err) + return m + } + + // A write into a mount point the image does not ship must not land in the layer + // either. Rootfully /sys is a read-only mount that rejects it, so rootlessly the + // mount point must not exist yet while the container runs. + execSt := base.Run(llb.Args([]string{"/bin/sh", "-c", "touch /sys/written || true"})) + + before := entries(t, base) + after := entries(t, execSt.Root()) + + var added, removed []string + for k := range after { + if _, ok := before[k]; !ok { + added = append(added, k) + } + } + for k := range before { + if _, ok := after[k]; !ok { + removed = append(removed, k) + } + } + + // Nothing the exec wrote may survive, so the only entries it adds are the runtime's + // own mount points. /etc is not among them: the resolv.conf and hosts stubs are + // cleaned up again, and so is the /etc that would have been created to hold them. + require.ElementsMatch(t, []string{"dev/", "proc/", "sys/"}, added) + require.Empty(t, removed) + + // Existing is not enough: a mount point that does not match what a rootful runtime + // creates still leaves the workers producing different images. + for _, p := range []string{"dev/", "proc/", "sys/"} { + hdr := after[p].Header + require.Equal(t, os.FileMode(0o755), hdr.FileInfo().Mode().Perm(), "mode of %q", p) + require.Equal(t, 0, hdr.Uid, "uid of %q", p) + require.Equal(t, 0, hdr.Gid, "gid of %q", p) + } +} + // testMountStubsTimestamp verifies that timestamps set on directories used as mount points // (and their parents) are preserved after the mount is removed. // https://github.com/moby/buildkit/issues/3148 diff --git a/client/client_test.go b/client/client_test.go index d3e66f24f9ae..3595db285dde 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -187,6 +187,7 @@ var allTests = []func(t *testing.T, sb integration.Sandbox){ testLLBMountPerformance, testLockedCacheMounts, testMountStubsDirectory, + testMountStubsRuntimeMountpoints, testMountStubsTimestamp, testMountWithNoSource, testRawSocketMount, diff --git a/executor/containerdexecutor/executor.go b/executor/containerdexecutor/executor.go index 69d0f447c887..44742cc66fbd 100644 --- a/executor/containerdexecutor/executor.go +++ b/executor/containerdexecutor/executor.go @@ -114,6 +114,9 @@ type containerState struct { // On Windows we need to use the root mounts to achieve the same thing that Linux does // with rootfsPath. So we save both in details. rootMounts []mount.Mount + // Destinations of the mounts that the rootless spec conversion removed, whose mount + // points have to be recreated after the container is gone. + removedMounts []string } func (w *containerdExecutor) Run(ctx context.Context, id string, root executor.Mount, mounts []executor.Mount, process executor.ProcessInfo, started chan<- struct{}) (rec resourcestypes.Recorder, err error) { @@ -208,6 +211,15 @@ func (w *containerdExecutor) Run(ctx context.Context, id string, root executor.M defer releaseSpec() } + // Recreate the mount points that the rootless spec conversion removed, so that they + // are left in the rootfs the way a rootful build leaves them. This executor has no + // identity mapping, so they are owned by root. moby/buildkit#6686 + defer func() { + if err == nil { + err = executor.CreateMountStubs(details.rootfsPath, details.removedMounts, 0, 0) + } + }() + opts := []ctd.NewContainerOpts{ ctd.WithSpec(spec), } diff --git a/executor/containerdexecutor/executor_unix.go b/executor/containerdexecutor/executor_unix.go index 328f0568c5e1..c30fa742c91e 100644 --- a/executor/containerdexecutor/executor_unix.go +++ b/executor/containerdexecutor/executor_unix.go @@ -159,10 +159,15 @@ func (w *containerdExecutor) createOCISpec(ctx context.Context, id, resolvConf, releasers = append(releasers, cleanup) spec.Process.Terminal = meta.Tty if w.rootless { - if err := rootlessspecconv.ToRootless(spec); err != nil { + removedMounts, err := rootlessspecconv.ToRootless(spec) + if err != nil { releaseAll() return nil, nil, err } + // The runtime no longer sets these mounts up, but a rootful build still gets + // their mount points left in the rootfs. The caller recreates them once the + // container is gone. moby/buildkit#6686 + details.removedMounts = removedMounts } return spec, releaseAll, nil } diff --git a/executor/runcexecutor/executor.go b/executor/runcexecutor/executor.go index 46e70ef02d81..eee61e29c2b9 100644 --- a/executor/runcexecutor/executor.go +++ b/executor/runcexecutor/executor.go @@ -335,9 +335,24 @@ func (w *runcExecutor) Run(ctx context.Context, id string, root executor.Mount, spec.Process.Terminal = meta.Tty spec.Process.OOMScoreAdj = w.oomScoreAdj if w.rootless { - if err := rootlessspecconv.ToRootless(spec); err != nil { + var removedMounts []string + removedMounts, err = rootlessspecconv.ToRootless(spec) + if err != nil { return nil, err } + // The runtime no longer sets these mounts up, but a rootful build still gets + // their mount points left in the rootfs. Recreate them once the container is + // gone: creating them up front would turn a mount point that the image does + // not ship into a directory the build can write to. moby/buildkit#6686 + var stubUID, stubGID int + if w.idmap != nil { + stubUID, stubGID = w.idmap.RootPair() + } + defer func() { + if err == nil { + err = executor.CreateMountStubs(rootFSPath, removedMounts, stubUID, stubGID) + } + }() } if err := json.NewEncoder(f).Encode(spec); err != nil { diff --git a/executor/stubs.go b/executor/stubs.go index d39be0fbe8d3..28cc8329495d 100644 --- a/executor/stubs.go +++ b/executor/stubs.go @@ -2,16 +2,50 @@ package executor import ( "context" - "errors" "os" "path/filepath" + "slices" "strings" "syscall" "github.com/containerd/continuity/fs" "github.com/moby/buildkit/util/bklog" + "github.com/moby/sys/user" + "github.com/pkg/errors" ) +// CreateMountStubs creates the directories that the container runtime would have created +// itself as mount points for dests. Destinations nested under another destination are +// skipped: the runtime sets the parent mount up first and creates those inside it, so +// they never become part of the rootfs. +// +// This is for mounts that were dropped from the spec before it reached the runtime, so +// that the mount points left in the rootfs do not depend on whether the mount was +// dropped. +func CreateMountStubs(dir string, dests []string, uid, gid int) error { + for _, dest := range dests { + p := filepath.Join("/", dest) + if p == "/" { + continue + } + if slices.ContainsFunc(dests, func(other string) bool { + other = filepath.Join("/", other) + return other != p && strings.HasPrefix(p, other+"/") + }) { + continue + } + realPath, err := fs.RootPath(dir, p) + if err != nil { + return errors.WithStack(err) + } + // WithOnlyNew leaves a directory that the image already provides alone. + if err := user.MkdirAllAndChown(realPath, 0o755, uid, gid, user.WithOnlyNew); err != nil { + return errors.Wrapf(err, "failed to create mount stub %q", p) + } + } + return nil +} + func MountStubsCleaner(ctx context.Context, dir string, mounts []Mount, recursive bool) func() { names := []string{"/etc/resolv.conf", "/etc/hosts"} diff --git a/executor/stubs_test.go b/executor/stubs_test.go new file mode 100644 index 000000000000..107c0ec4bac5 --- /dev/null +++ b/executor/stubs_test.go @@ -0,0 +1,48 @@ +//go:build !windows + +package executor + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestCreateMountStubs(t *testing.T) { + for _, tc := range []struct { + name string + dests []string + }{ + {name: "parent first", dests: []string{"/sys", "/sys/fs/cgroup"}}, + {name: "child first", dests: []string{"/sys/fs/cgroup", "/sys"}}, + } { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + require.NoError(t, CreateMountStubs(root, tc.dests, os.Getuid(), os.Getgid())) + + st, err := os.Stat(filepath.Join(root, "sys")) + require.NoError(t, err) + require.True(t, st.IsDir()) + + // The runtime creates this one inside the /sys mount, so it must not end + // up in the rootfs. + _, err = os.Lstat(filepath.Join(root, "sys/fs")) + require.ErrorIs(t, err, os.ErrNotExist) + }) + } + + t.Run("keeps existing directory", func(t *testing.T) { + root := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(root, "sys"), 0o700)) + require.NoError(t, os.WriteFile(filepath.Join(root, "sys/keep"), []byte("keep"), 0o644)) + + require.NoError(t, CreateMountStubs(root, []string{"/sys"}, os.Getuid(), os.Getgid())) + + st, err := os.Stat(filepath.Join(root, "sys")) + require.NoError(t, err) + require.Equal(t, os.FileMode(0o700), st.Mode().Perm()) + require.FileExists(t, filepath.Join(root, "sys/keep")) + }) +} diff --git a/util/rootless/specconv/specconv_linux.go b/util/rootless/specconv/specconv_linux.go index 7118f8d6d2aa..ae0c06183e59 100644 --- a/util/rootless/specconv/specconv_linux.go +++ b/util/rootless/specconv/specconv_linux.go @@ -10,8 +10,12 @@ import ( // * Remove /sys mount // * Remove cgroups // +// It returns the destinations of the mounts it removed. A rootful runtime creates these +// directories in the rootfs while setting the mounts up, so the caller needs to create +// them as well for the mount points left behind not to depend on the rootless mode. +// // See docs/rootless.md for the supported runc revision. -func ToRootless(spec *specs.Spec) error { +func ToRootless(spec *specs.Spec) ([]string, error) { // Remove /sys mount because we can't mount /sys when the daemon netns // is not unshared from the host. // @@ -25,8 +29,10 @@ func ToRootless(spec *specs.Spec) error { // For buildkit usecase, we suppose we don't need to provide /sys to // containers and remove /sys mount as a workaround. var mounts []specs.Mount + var removed []string for _, mount := range spec.Mounts { if strings.HasPrefix(mount.Destination, "/sys") { + removed = append(removed, mount.Destination) continue } mounts = append(mounts, mount) @@ -36,5 +42,5 @@ func ToRootless(spec *specs.Spec) error { // Remove cgroups so as to avoid `container_linux.go:337: starting container process caused "process_linux.go:280: applying cgroup configuration for process caused \"mkdir /sys/fs/cgroup/cpuset/buildkit: permission denied\""` spec.Linux.Resources = nil spec.Linux.CgroupsPath = "" - return nil + return removed, nil } diff --git a/util/rootless/specconv/specconv_nonlinux.go b/util/rootless/specconv/specconv_nonlinux.go index c200057e319e..fdf5a383c04c 100644 --- a/util/rootless/specconv/specconv_nonlinux.go +++ b/util/rootless/specconv/specconv_nonlinux.go @@ -14,6 +14,6 @@ import ( // * Remove cgroups // // See docs/rootless.md for the supported runc revision. -func ToRootless(spec *specs.Spec) error { - return errors.Errorf("not implemented on on %s", runtime.GOOS) +func ToRootless(spec *specs.Spec) ([]string, error) { + return nil, errors.Errorf("not implemented on %s", runtime.GOOS) }