From 069b2d673b4cba9fb195e8229b93432947d79ace Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 12 Aug 2026 08:47:58 +0800 Subject: [PATCH 1/2] executor: clean finalized runc mount stubs BuildKit records mount-stub cleanup before rootless conversion finishes changing the mount list, which can leave rootful and rootless builds with different empty directories. This change registers cleanup after rootless conversion, when the mount list is final, and uses that list for cleanup. Signed-off-by: Leo Li --- executor/runcexecutor/executor.go | 3 ++- executor/stubs_spec_linux.go | 18 ++++++++++++++++ executor/stubs_spec_linux_test.go | 34 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 executor/stubs_spec_linux.go create mode 100644 executor/stubs_spec_linux_test.go diff --git a/executor/runcexecutor/executor.go b/executor/runcexecutor/executor.go index 46e70ef02d81..24bca6d2ced5 100644 --- a/executor/runcexecutor/executor.go +++ b/executor/runcexecutor/executor.go @@ -277,7 +277,6 @@ func (w *runcExecutor) Run(ctx context.Context, id string, root executor.Mount, } defer mount.Unmount(rootFSPath, 0) - defer executor.MountStubsCleaner(context.WithoutCancel(ctx), rootFSPath, mounts, meta.RemoveMountStubsRecursive)() if proxyNS, ok := namespace.(network.ProxyNamespace); ok { cleanProxyCA, err := executor.InjectProxyCA(rootFSPath, proxyNS.ProxyCACert()) if err != nil { @@ -340,6 +339,8 @@ func (w *runcExecutor) Run(ctx context.Context, id string, root executor.Mount, } } + defer executor.MountStubsCleanerForSpec(context.WithoutCancel(ctx), rootFSPath, spec.Mounts, meta.RemoveMountStubsRecursive)() + if err := json.NewEncoder(f).Encode(spec); err != nil { return nil, errors.WithStack(err) } diff --git a/executor/stubs_spec_linux.go b/executor/stubs_spec_linux.go new file mode 100644 index 000000000000..2efd8b2b6e3f --- /dev/null +++ b/executor/stubs_spec_linux.go @@ -0,0 +1,18 @@ +//go:build linux + +package executor + +import ( + "context" + + specs "github.com/opencontainers/runtime-spec/specs-go" +) + +// MountStubsCleanerForSpec cleans stubs for mounts in a finalized OCI spec. +func MountStubsCleanerForSpec(ctx context.Context, dir string, mounts []specs.Mount, recursive bool) func() { + cleanupMounts := make([]Mount, len(mounts)) + for i, m := range mounts { + cleanupMounts[i].Dest = m.Destination + } + return MountStubsCleaner(ctx, dir, cleanupMounts, recursive) +} diff --git a/executor/stubs_spec_linux_test.go b/executor/stubs_spec_linux_test.go new file mode 100644 index 000000000000..99011bed0857 --- /dev/null +++ b/executor/stubs_spec_linux_test.go @@ -0,0 +1,34 @@ +//go:build linux + +package executor + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + + specs "github.com/opencontainers/runtime-spec/specs-go" +) + +func TestMountStubsCleanerForSpec(t *testing.T) { + root := t.TempDir() + clean := MountStubsCleanerForSpec(context.Background(), root, []specs.Mount{ + {Destination: "/proc"}, + }, true) + + for _, path := range []string{"proc", "sys"} { + if err := os.MkdirAll(filepath.Join(root, path), 0o755); err != nil { + t.Fatal(err) + } + } + clean() + + if _, err := os.Lstat(filepath.Join(root, "proc")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("runtime-created /proc survived cleanup: %v", err) + } + if _, err := os.Stat(filepath.Join(root, "sys")); err != nil { + t.Fatalf("rootless user-owned /sys was removed: %v", err) + } +} From 6ed70f8651124b07578deab2d427758474902773 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Thu, 13 Aug 2026 16:04:16 +0300 Subject: [PATCH 2/2] executor: restore mount points removed for rootless The rootless spec conversion removes the /sys mount, so the runtime never creates its mount point in the rootfs. A rootful build gets that empty directory, which meant the same LLB left different mount points behind depending on whether the worker was rootless. Recreate the mount points of the removed mounts after the container has exited, rather than cleaning them up from the rootful result, which would change existing output and need a compatibility version bump. Creating them up front would instead turn a mount point that the image does not ship into a directory the build can write to. Nested destinations are skipped: the runtime creates those inside the parent mount, where they never reach the rootfs. Fixes #6686 Signed-off-by: Tonis Tiigi --- client/client_mount_test.go | 85 ++++++++++++++++++++ client/client_test.go | 1 + executor/containerdexecutor/executor.go | 12 +++ executor/containerdexecutor/executor_unix.go | 7 +- executor/runcexecutor/executor.go | 20 ++++- executor/stubs.go | 36 ++++++++- executor/stubs_spec_linux.go | 18 ----- executor/stubs_spec_linux_test.go | 34 -------- executor/stubs_test.go | 48 +++++++++++ util/rootless/specconv/specconv_linux.go | 10 ++- util/rootless/specconv/specconv_nonlinux.go | 4 +- 11 files changed, 214 insertions(+), 61 deletions(-) delete mode 100644 executor/stubs_spec_linux.go delete mode 100644 executor/stubs_spec_linux_test.go create mode 100644 executor/stubs_test.go 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 24bca6d2ced5..eee61e29c2b9 100644 --- a/executor/runcexecutor/executor.go +++ b/executor/runcexecutor/executor.go @@ -277,6 +277,7 @@ func (w *runcExecutor) Run(ctx context.Context, id string, root executor.Mount, } defer mount.Unmount(rootFSPath, 0) + defer executor.MountStubsCleaner(context.WithoutCancel(ctx), rootFSPath, mounts, meta.RemoveMountStubsRecursive)() if proxyNS, ok := namespace.(network.ProxyNamespace); ok { cleanProxyCA, err := executor.InjectProxyCA(rootFSPath, proxyNS.ProxyCACert()) if err != nil { @@ -334,13 +335,26 @@ 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) + } + }() } - defer executor.MountStubsCleanerForSpec(context.WithoutCancel(ctx), rootFSPath, spec.Mounts, meta.RemoveMountStubsRecursive)() - if err := json.NewEncoder(f).Encode(spec); err != nil { return nil, errors.WithStack(err) } 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_spec_linux.go b/executor/stubs_spec_linux.go deleted file mode 100644 index 2efd8b2b6e3f..000000000000 --- a/executor/stubs_spec_linux.go +++ /dev/null @@ -1,18 +0,0 @@ -//go:build linux - -package executor - -import ( - "context" - - specs "github.com/opencontainers/runtime-spec/specs-go" -) - -// MountStubsCleanerForSpec cleans stubs for mounts in a finalized OCI spec. -func MountStubsCleanerForSpec(ctx context.Context, dir string, mounts []specs.Mount, recursive bool) func() { - cleanupMounts := make([]Mount, len(mounts)) - for i, m := range mounts { - cleanupMounts[i].Dest = m.Destination - } - return MountStubsCleaner(ctx, dir, cleanupMounts, recursive) -} diff --git a/executor/stubs_spec_linux_test.go b/executor/stubs_spec_linux_test.go deleted file mode 100644 index 99011bed0857..000000000000 --- a/executor/stubs_spec_linux_test.go +++ /dev/null @@ -1,34 +0,0 @@ -//go:build linux - -package executor - -import ( - "context" - "errors" - "os" - "path/filepath" - "testing" - - specs "github.com/opencontainers/runtime-spec/specs-go" -) - -func TestMountStubsCleanerForSpec(t *testing.T) { - root := t.TempDir() - clean := MountStubsCleanerForSpec(context.Background(), root, []specs.Mount{ - {Destination: "/proc"}, - }, true) - - for _, path := range []string{"proc", "sys"} { - if err := os.MkdirAll(filepath.Join(root, path), 0o755); err != nil { - t.Fatal(err) - } - } - clean() - - if _, err := os.Lstat(filepath.Join(root, "proc")); !errors.Is(err, os.ErrNotExist) { - t.Fatalf("runtime-created /proc survived cleanup: %v", err) - } - if _, err := os.Stat(filepath.Join(root, "sys")); err != nil { - t.Fatalf("rootless user-owned /sys was removed: %v", err) - } -} 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) }