Merge pull request #7039 from tonistiigi/restore-rootless-mountpoints

executor: restore mount points removed for rootless

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

client/client_test.go

@@ -188,6 +188,7 @@ var allTests = []func(t *testing.T, sb integration.Sandbox){
 	testLLBMountPerformance,
 	testLockedCacheMounts,
 	testMountStubsDirectory,
+	testMountStubsRuntimeMountpoints,
 	testMountStubsTimestamp,
 	testMountWithNoSource,
 	testRawSocketMount,

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),
 	}

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
 }

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 {

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"}
 

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"))
+	})
+}

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
 }

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)
 }