Fix leaking of tmpfs mount in CDI mode #1168
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Pull Request Test Coverage Report for Build 16051212578Details
💛 - Coveralls |
ArangoGutierrez
force-pushed
the
b/5363680
branch
4 times, most recently
from
38c25d5 to
1c79350
Compare
elezar
reviewed
Jun 30, 2025
| return unix.Mount("tmpfs", target, "tmpfs", 0, fmt.Sprintf("size=%d", size)) | ||
| } | ||
|
|
||
| func createFileInRoot(containerRootDirPath string, destinationPath string, mode os.FileMode) (string, error) { |
There was a problem hiding this comment.
TODO: This function also exists in internal/ldconfig we should move this to a separate pacakge.
elezar
reviewed
Jun 30, 2025
| _, _, err = runner.Run("mkdir -p /tmp/empty") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| _, _, err = runner.Run("mount | sort > /tmp/mounts.before") |
There was a problem hiding this comment.
What about capturing the output of mount | sort as variables and then comparing these using the Gomega matchers?
elezar
reviewed
Jun 30, 2025
| Expect(output).To(Equal("ModifyDeviceFiles: 0\n")) | ||
| }) | ||
|
|
||
| //sudo docker run --runtime=nvidia --rm -ti -e NVIDIA_VISIBLE_DEVICES=all -e NVIDIA_DRIVER_CAPABILITIES=all --mount type=bind,source=/tmp/empty,target=/empty,bind-propagation=shared ubuntu true |
There was a problem hiding this comment.
What is the purpose of this comment?
There was a problem hiding this comment.
oh, sorry it was me during devel, to not forget that bit
elezar
reviewed
Jun 30, 2025
| }) | ||
|
|
||
| //sudo docker run --runtime=nvidia --rm -ti -e NVIDIA_VISIBLE_DEVICES=all -e NVIDIA_DRIVER_CAPABILITIES=all --mount type=bind,source=/tmp/empty,target=/empty,bind-propagation=shared ubuntu true | ||
| It("should work with nvidia-container-runtime", func(ctx context.Context) { |
There was a problem hiding this comment.
Let's also add a case for --runtime=runc for the "legacy" code path. As a general question, could we add a tag to tests to indicate that it's targeting the legacy code path?
elezar
reviewed
Jun 30, 2025
| BeforeAll(func(ctx context.Context) { | ||
| _, _, err := runner.Run("docker pull ubuntu") | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
There was a problem hiding this comment.
Nit: Let's remove this from the diff.
ArangoGutierrez
changed the title
ArangoGutierrez
changed the title
ArangoGutierrez
force-pushed
the
b/5363680
branch
2 times, most recently
from
50ac666 to
91f1a73
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how the NVIDIA container runtime hook creates and mounts its params file—switching to procfd-based APIs and secure file creation to prevent mount leaks—and adds end-to-end tests to ensure no host mounts remain after running a container.
- Switch
createParamsFileInContainerto useutils.WithProcfdand acreateFileInRoothelper for safer tmpfs and bind mounts. - Introduce a secure, mknodat-based file creation function (
createFileInRoot). - Add E2E tests that record host mounts before/after running containers in both legacy and nvidia runtimes to catch mount leaks.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/e2e/nvidia-container-toolkit_test.go | New “Disabling device node creation” suite: captures host mounts, runs containers, and asserts no new mounts |
| cmd/nvidia-cdi-hook/disable-device-node-modification/params_linux.go | Refactored createParamsFileInContainer to use procfd mounts and secure file creation, replacing temp dir logic |
Comments suppressed due to low confidence (1)
cmd/nvidia-cdi-hook/disable-device-node-modification/params_linux.go:44
- [nitpick] The error message could include the target path (e.g.,
hookScratchDirPath) to make debugging mount failures clearer.
return fmt.Errorf("failed to create tmpfs mount for params file: %w", err)
91f1a73 to
70cb383
Compare
This change adds e2e tests to ensure that running a container with shared mount propagaion does not result in leaked mounts. Note that as soon as a single bind mount is requested with shared propagation, the rootfs is also mounted with shared propagation. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Evan Lezar <elezar@nvidia.com>
This change ensures that the tmpfs mount created for the modified NVIDIA params file does not leak to the host. Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com> Co-authored-by: Evan Lezar <elezar@nvidia.com>
ArangoGutierrez
force-pushed
the
b/5363680
branch
from
bc9ebdd to
6d68eb7
Compare
elezar
approved these changes
Jul 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch fixes the handling of temporary files and directories in the NVIDIA container runtime hook, ensuring we don't leak tmpfs mounts when handling the params file in the container when in CDI mode. This is done by ensuring that the tmpfs mount that we create for the modified params file is created in the container's mount namespace instead of on the host.
We also add end2end tests to check for leaking of mounts.