Merge pull request #1168 from ArangoGutierrez/b/5363680

Fix leaking of tmpfs mount in CDI mode

Author: elezar

cmd/nvidia-cdi-hook/disable-device-node-modification/params_linux.go

@@ -20,39 +20,57 @@
 package disabledevicenodemodification
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 
+	securejoin "github.com/cyphar/filepath-securejoin"
 	"github.com/opencontainers/runc/libcontainer/utils"
 	"golang.org/x/sys/unix"
 )
 
 func createParamsFileInContainer(containerRootDirPath string, contents []byte) error {
-	tmpRoot, err := os.MkdirTemp("", "nvct-empty-dir*")
-	if err != nil {
-		return fmt.Errorf("failed to create temp root: %w", err)
+	hookScratchDirPath := "/var/run/nvidia-ctk-hook"
+	if err := utils.MkdirAllInRoot(containerRootDirPath, hookScratchDirPath, 0755); err != nil {
+		return fmt.Errorf("error creating hook scratch folder: %w", err)
 	}
 
-	if err := createTmpFs(tmpRoot, len(contents)); err != nil {
-		return fmt.Errorf("failed to create tmpfs mount for params file: %w", err)
-	}
+	err := utils.WithProcfd(containerRootDirPath, hookScratchDirPath, func(hookScratchDirFdPath string) error {
+		return createTmpFs(hookScratchDirFdPath, len(contents))
 
-	modifiedParamsFile, err := os.OpenFile(filepath.Join(tmpRoot, "nvct-params"), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0444)
+	})
 	if err != nil {
-		return fmt.Errorf("failed to open modified params file: %w", err)
+		return fmt.Errorf("failed to create tmpfs mount for params file: %w", err)
 	}
-	defer modifiedParamsFile.Close()
 
-	if _, err := modifiedParamsFile.Write(contents); err != nil {
-		return fmt.Errorf("failed to write temporary params file: %w", err)
+	modifiedParamsFilePath := filepath.Join(hookScratchDirPath, "nvct-params")
+	if _, err := createFileInRoot(containerRootDirPath, modifiedParamsFilePath, 0444); err != nil {
+		return fmt.Errorf("error creating modified params file: %w", err)
 	}
 
-	err = utils.WithProcfd(containerRootDirPath, nvidiaDriverParamsPath, func(nvidiaDriverParamsFdPath string) error {
-		return unix.Mount(modifiedParamsFile.Name(), nvidiaDriverParamsFdPath, "", unix.MS_BIND|unix.MS_RDONLY|unix.MS_NODEV|unix.MS_PRIVATE|unix.MS_NOSYMFOLLOW, "")
+	err = utils.WithProcfd(containerRootDirPath, modifiedParamsFilePath, func(modifiedParamsFileFdPath string) error {
+		modifiedParamsFile, err := os.OpenFile(modifiedParamsFileFdPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0444)
+		if err != nil {
+			return fmt.Errorf("failed to open modified params file: %w", err)
+		}
+		defer modifiedParamsFile.Close()
+
+		if _, err := modifiedParamsFile.Write(contents); err != nil {
+			return fmt.Errorf("failed to write temporary params file: %w", err)
+		}
+
+		err = utils.WithProcfd(containerRootDirPath, nvidiaDriverParamsPath, func(nvidiaDriverParamsFdPath string) error {
+			return unix.Mount(modifiedParamsFileFdPath, nvidiaDriverParamsFdPath, "", unix.MS_BIND|unix.MS_RDONLY|unix.MS_NODEV|unix.MS_PRIVATE|unix.MS_NOSYMFOLLOW, "")
+		})
+		if err != nil {
+			return fmt.Errorf("failed to mount modified params file: %w", err)
+		}
+
+		return nil
 	})
 	if err != nil {
-		return fmt.Errorf("failed to mount modified params file: %w", err)
+		return err
 	}
 
 	return nil
@@ -61,3 +79,32 @@ func createParamsFileInContainer(containerRootDirPath string, contents []byte) e
 func createTmpFs(target string, size int) error {
 	return unix.Mount("tmpfs", target, "tmpfs", 0, fmt.Sprintf("size=%d", size))
 }
+
+// TODO(ArangoGutierrez): This function also exists in internal/ldconfig we should move this to a separate package.
+func createFileInRoot(containerRootDirPath string, destinationPath string, mode os.FileMode) (string, error) {
+	dest, err := securejoin.SecureJoin(containerRootDirPath, destinationPath)
+	if err != nil {
+		return "", err
+	}
+	// Make the parent directory.
+	destDir, destBase := filepath.Split(dest)
+	destDirFd, err := utils.MkdirAllInRootOpen(containerRootDirPath, destDir, 0755)
+	if err != nil {
+		return "", fmt.Errorf("error creating parent dir: %w", err)
+	}
+	defer destDirFd.Close()
+	// Make the target file. We want to avoid opening any file that is
+	// already there because it could be a "bad" file like an invalid
+	// device or hung tty that might cause a DoS, so we use mknodat.
+	// destBase does not contain any "/" components, and mknodat does
+	// not follow trailing symlinks, so we can safely just call mknodat
+	// here.
+	if err := unix.Mknodat(int(destDirFd.Fd()), destBase, unix.S_IFREG|uint32(mode), 0); err != nil {
+		// If we get EEXIST, there was already an inode there and
+		// we can consider that a success.
+		if !errors.Is(err, unix.EEXIST) {
+			return "", fmt.Errorf("error creating empty file: %w", err)
+		}
+	}
+	return dest, nil
+}

tests/e2e/nvidia-container-toolkit_test.go

@@ -217,7 +217,7 @@ var _ = Describe("docker", Ordered, ContinueOnFailure, func() {
 		})
 	})
 
-	Describe("Disabling device node creation", Ordered, func() {
+	When("Disabling device node creation", Ordered, func() {
 		BeforeAll(func(ctx context.Context) {
 			_, _, err := runner.Run("docker pull ubuntu")
 			Expect(err).ToNot(HaveOccurred())
@@ -257,4 +257,44 @@ var _ = Describe("docker", Ordered, ContinueOnFailure, func() {
 			Expect(libs).To(ContainElements([]string{"libcuda.so", "libcuda.so.1"}))
 		})
 	})
+
+	When("Running containers with shared mount propagation", Ordered, func() {
+		var mountsBefore string
+		var tmpDirPath string
+
+		BeforeAll(func(ctx context.Context) {
+			_, _, err := runner.Run("docker pull ubuntu")
+			Expect(err).ToNot(HaveOccurred())
+
+			tmpDirPath = GinkgoT().TempDir()
+			_, _, err = runner.Run("mkdir -p " + tmpDirPath)
+			Expect(err).ToNot(HaveOccurred())
+
+			output, _, err := runner.Run("mount | sort")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(output).ToNot(BeEmpty())
+			mountsBefore = output
+		})
+
+		AfterEach(func() {
+			mountsAfter, _, err := runner.Run("mount | sort")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(mountsAfter).To(Equal(mountsBefore))
+		})
+
+		It("should not leak mounts when using the nvidia-container-runtime-hook", Label("legacy"), func(ctx context.Context) {
+			_, _, err := runner.Run("docker run --rm -i --runtime=runc -e NVIDIA_VISIBLE_DEVICES=all -e NVIDIA_DRIVER_CAPABILITIES=all --mount type=bind,source=" + tmpDirPath + ",target=/empty,bind-propagation=shared ubuntu true")
+			Expect(err).ToNot(HaveOccurred())
+		})
+
+		It("should not leak mounts when using the nvidia-container-runtime", func(ctx context.Context) {
+			_, _, err := runner.Run("docker run --rm -i --runtime=nvidia -e NVIDIA_VISIBLE_DEVICES=all -e NVIDIA_DRIVER_CAPABILITIES=all --mount type=bind,source=" + tmpDirPath + ",target=/empty,bind-propagation=shared ubuntu true")
+			Expect(err).ToNot(HaveOccurred())
+		})
+
+		It("should not leak mounts when using CDI mode", func(ctx context.Context) {
+			_, _, err := runner.Run("docker run --rm -i --runtime=nvidia -e NVIDIA_VISIBLE_DEVICES=all -e NVIDIA_DRIVER_CAPABILITIES=runtime.nvidia.com/gpu=all --mount type=bind,source=" + tmpDirPath + ",target=/empty,bind-propagation=shared ubuntu true")
+			Expect(err).ToNot(HaveOccurred())
+		})
+	})
 })