From 54953f2f5a5157a9ae92d25831a2b77c20cfab82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 10 Aug 2023 19:13:38 +0200 Subject: [PATCH 1/3] integration: Add test for not breaking overlayfs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check that operations that could potentially perform overlayfs mounts that could cause undefined behaviors. Signed-off-by: Paweł Gronowski (cherry picked from commit 303e2b124e6697a232d0c1a5207cddd79e561fe1) Signed-off-by: Paweł Gronowski --- integration/container/overlayfs_linux_test.go | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 integration/container/overlayfs_linux_test.go diff --git a/integration/container/overlayfs_linux_test.go b/integration/container/overlayfs_linux_test.go new file mode 100644 index 0000000000000..4736e6afc22f1 --- /dev/null +++ b/integration/container/overlayfs_linux_test.go @@ -0,0 +1,111 @@ +package container + +import ( + "context" + "io" + "strings" + "testing" + + "github.com/docker/docker/api/types" + "github.com/docker/docker/integration/internal/container" + "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/dmesg" + "gotest.tools/v3/assert" + "gotest.tools/v3/skip" +) + +func TestNoOverlayfsWarningsAboutUndefinedBehaviors(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType != "linux", "overlayfs is only available on linux") + skip.If(t, testEnv.IsRemoteDaemon(), "local daemon is needed for kernel log access") + skip.If(t, testEnv.IsRootless(), "root is needed for reading kernel log") + + defer setupTest(t)() + client := testEnv.APIClient() + ctx := context.Background() + + cID := container.Run(ctx, t, client, container.WithCmd("sh", "-c", `while true; do echo $RANDOM >>/file; sleep 0.1; done`)) + + testCases := []struct { + name string + operation func(t *testing.T) error + }{ + {name: "diff", operation: func(*testing.T) error { + _, err := client.ContainerDiff(ctx, cID) + return err + }}, + {name: "export", operation: func(*testing.T) error { + rc, err := client.ContainerExport(ctx, cID) + if err == nil { + defer rc.Close() + _, err = io.Copy(io.Discard, rc) + } + return err + }}, + {name: "cp to container", operation: func(t *testing.T) error { + archive, err := archive.Generate("new-file", "hello-world") + assert.NilError(t, err, "failed to create a temporary archive") + return client.CopyToContainer(ctx, cID, "/", archive, types.CopyToContainerOptions{}) + }}, + {name: "cp from container", operation: func(*testing.T) error { + rc, _, err := client.CopyFromContainer(ctx, cID, "/file") + if err == nil { + defer rc.Close() + _, err = io.Copy(io.Discard, rc) + } + + return err + }}, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + prev := dmesgLines(256) + + err := tc.operation(t) + assert.NilError(t, err) + + after := dmesgLines(2048) + + diff := diffDmesg(prev, after) + for _, line := range diff { + overlayfs := strings.Contains(line, "overlayfs: ") + lowerDirInUse := strings.Contains(line, "lowerdir is in-use as ") + upperDirInUse := strings.Contains(line, "upperdir is in-use as ") + workDirInuse := strings.Contains(line, "workdir is in-use as ") + undefinedBehavior := strings.Contains(line, "will result in undefined behavior") + + if overlayfs && (lowerDirInUse || upperDirInUse || workDirInuse) && undefinedBehavior { + t.Errorf("%s caused overlayfs kernel warning: %s", tc.name, line) + } + } + }) + } +} + +func dmesgLines(bytes int) []string { + data := dmesg.Dmesg(bytes) + return strings.Split(strings.TrimSpace(string(data)), "\n") +} + +func diffDmesg(prev, next []string) []string { + // All lines have a timestamp, so just take the last one from the previous + // log and find it in the new log. + lastPrev := prev[len(prev)-1] + + for idx := len(next) - 1; idx >= 0; idx-- { + line := next[idx] + + if line == lastPrev { + nextIdx := idx + 1 + if nextIdx < len(next) { + return next[nextIdx:] + } else { + // Found at the last position, log is the same. + return nil + } + } + } + + return next +} From b76a0c7d009c9859eb01fce680685cb55b4a36bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 10 Aug 2023 15:33:06 +0200 Subject: [PATCH 2/3] c8d/export: Use ref counted mounter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To prevent mounting the container rootfs in a rw mode if it's already mounted. This can't use `mount.WithReadonlyTempMount` because the archive code does a chroot with a pivot_root, which creates a new directory in the rootfs. Signed-off-by: Paweł Gronowski (cherry picked from commit 051d51b22212bb570998e8cf3593036177c4a647) Signed-off-by: Paweł Gronowski --- daemon/containerd/image_exporter.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/daemon/containerd/image_exporter.go b/daemon/containerd/image_exporter.go index 21bbcfc48ef4c..9dfb3e6a0ec73 100644 --- a/daemon/containerd/image_exporter.go +++ b/daemon/containerd/image_exporter.go @@ -11,7 +11,6 @@ import ( containerdimages "github.com/containerd/containerd/images" "github.com/containerd/containerd/images/archive" "github.com/containerd/containerd/leases" - "github.com/containerd/containerd/mount" cplatforms "github.com/containerd/containerd/platforms" "github.com/docker/distribution/reference" "github.com/docker/docker/container" @@ -30,7 +29,13 @@ func (i *ImageService) PerformWithBaseFS(ctx context.Context, c *container.Conta if err != nil { return err } - return mount.WithTempMount(ctx, mounts, fn) + path, err := i.refCountMounter.Mount(mounts, c.ID) + if err != nil { + return err + } + defer i.refCountMounter.Unmount(path) + + return fn(path) } // ExportImage exports a list of images to the given output stream. The From 74bf46aea6e17fe088e5efa6647c4fb558b22398 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 10 Aug 2023 19:05:03 +0200 Subject: [PATCH 3/3] c8d/diff: Reuse mount, mount parent as read-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The container rw layer may already be mounted, so it's not safe to use it in another overlay mount. Use the ref counted mounter (which will reuse the existing mount if it exists) to avoid that. Also, mount the parent mounts (layers of the base image) in a read-only mode. Signed-off-by: Paweł Gronowski (cherry picked from commit 6da42ca8308fccf3ae5be75d599eeda61f7e41ed) Signed-off-by: Paweł Gronowski --- daemon/containerd/image_changes.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/daemon/containerd/image_changes.go b/daemon/containerd/image_changes.go index 94c56cf6161c6..dd44aebe80717 100644 --- a/daemon/containerd/image_changes.go +++ b/daemon/containerd/image_changes.go @@ -58,15 +58,10 @@ func (i *ImageService) Changes(ctx context.Context, container *container.Contain } }() - mounts, err := snapshotter.Mounts(ctx, container.ID) - if err != nil { - return nil, err - } - var changes []archive.Change - err = mount.WithReadonlyTempMount(ctx, mounts, func(fs string) error { - return mount.WithTempMount(ctx, parent, func(root string) error { - changes, err = archive.ChangesDirs(fs, root) + err = i.PerformWithBaseFS(ctx, container, func(containerRootfs string) error { + return mount.WithReadonlyTempMount(ctx, parent, func(parentRootfs string) error { + changes, err = archive.ChangesDirs(containerRootfs, parentRootfs) return err }) })