Repository navigation
Conversation
elezar
added
test:e2e
|
Label |
|
Label |
elezar
added
test:e2e
|
Label |
|
Label |
|
🌿 Preview your docs: https://nvidia-preview-pr-4204.docs.buildwithfern.com/openshell |
|
The new CDI spec bind mounts need to participate in the existing GPU trust exemption. With resource admission enabled by default, We can treat GPU attachments and their driver-managed CDI spec projections as trusted. Please have admission recognize and allow the exact expected CDI mounts by validating their daemon-reported source directory, reserved workload destination, and read-only mode. Keep rejecting arbitrary host binds, including on GPU sandboxes, rather than skipping admission for the entire container. Please also add a GPU provisioning/admission regression test with resource admission enabled; the current GPU test configuration disables it. |
drew
left a comment
drew
left a comment
There was a problem hiding this comment.
Reviewed at 966d8ea. I found two validation issues, detailed inline: library-parent expansion bypasses broad-path rejection, and implicitly writable mounts skip the new opt-in checks. Please add regression cases for both.
The overall design keeps Docker responsible for device injection and resolves policy requirements inside the workload namespace.
The Docker troubleshooting skill (skills/debug-openshell-cluster/SKILL.md) also needs a companion update covering CDI projection and supplemental-group failures.
Validation: static review only; I did not run tests. CI was still running when checked, and the PR description reports Docker GPU E2E validation as pending.
| for (path, access) in self.mount_paths { | ||
| match access { | ||
| CdiAccess::ReadOnly => { | ||
| read_only_paths.insert(shared_library_parent(&path).unwrap_or(path)); |
There was a problem hiding this comment.
Validate the derived library parent before adding it. Before this PR, GPU enrichment used a fixed allowlist. Now a read-only CDI destination such as /libexample.so passes normalize_policy_path() and then expands here to /, granting read access far beyond that library within the outer sandbox boundary. validate_cdi_requirements() only checks that the resulting directory exists, so it does not catch this. Apply the broad-path rejection to the derived parent as well, and add a regression case for a library directly beneath / (and other rejected roots).
| if read_write_requested { | ||
| Ok(CdiAccess::ReadWrite) | ||
| } else { | ||
| Ok(CdiAccess::ReadOnly) |
There was a problem hiding this comment.
Do not infer read-only access from the absence of rw. This classifies options: [bind] or [rbind] as read-only, although a bind mount can preserve a writable source mount. Consequently, these mounts never reach the single-file and exact-path allowlist checks. For example, a writable CDI directory mounted at /tmp/device-data remains writable under an existing /tmp read-write policy, despite the new contract rejecting writable CDI directories. Require explicit read-only semantics or conservatively validate the mount as writable, with regression coverage for omitted ro/rw options. See Linux bind-mount semantics.
|
Thanks for the CDI work. We have an IGX Orin JetPack 6.2 reproduction that appears relevant: the non-root workload lacks group 44 and The host reports NVIDIA Container Toolkit in CSV mode. From this PR's current diff, I understand the Docker path requires daemon-reported CDI spec directories and derives device and supplemental-group requirements from the selected spec. We're checking whether this host exposes those prerequisites; CSV mode alone does not answer that. Is this IGX configuration within the intended scope of #4204, and would a hardware retest be useful once the PR is ready? We can provide the host's |
I would say that this IGX configuration is within the intended scope of this PR. (In addition to #2846 which addresses the location of Some things to note:
|
966d8ea to
4cad83e
Compare
krishicks
left a comment
krishicks
left a comment
There was a problem hiding this comment.
🤖 This review was written by Claude (an AI assistant), working commit by commit through the PR at
d87c6abb7cwith @krishicks, who asked for it and is posting it. Findings come from reading the diffs againstmain; nothing was executed, so please verify before acting on them.
The descriptor-anchored validation in cdi_paths.rs is careful work, and the symlink-swap tests are a nice touch. Below are the issues that seem worth addressing, roughly in priority order, followed by an optional suggestion on structure.
Issues
1. CDI escalates the entire Landlock ruleset to HardRequirement, not just CDI rules (landlock.rs, prepare_with_path_open_mode)
The overridden compatibility also drives the availability probe and try_open_path for authored paths. For every CDI GPU sandbox, a missing authored path (e.g. a default-policy directory absent from the image) and a kernel/runtime without Landlock both become fatal. The docs describe this as "CDI validation and enforcement failures abort launch", which undersells it. Could this be scoped to the CDI targets only, or called out explicitly as a policy decision?
2. CDI spec-dir binds have host-side effects (DockerGpuRuntimeCapabilities::cdi_spec_bind_strings)
The projection builds "{source}:{dst}:ro,z" directly instead of going through docker_bind_string:
zis forced, so every GPU sandbox relabels the host's/etc/cdi//var/run/cdi. User binds only relabel on explicitselinux_labelopt-in.- The existence check is skipped.
docker_bind_stringchecks existence precisely because legacy-vbinds silently create missing sources. Docker commonly reports both/etc/cdiand/var/run/cdiwhen only one exists, so the other gets created as an empty root-owned host directory.
Relatedly, the whole spec directory (every vendor's specs) is exposed to the workload, not just the selected devices. Landlock likely keeps agents out of /run/openshell/boundary, but it would be good to confirm and note that.
3. The fixture baselines exercise a code path production doesn't use (cdi_linux.rs tests)
The three NVIDIA fixture baselines and most behavioral tests go through resolve_spec_hints, which is #[cfg(test)] and classifies access from spec ro/rw options. Production resolve_cdi_context deliberately ignores those and classifies from effective mount state. The two have already drifted: the Orin and Spark baselines still list /run/nvidia-persistenced/socket under read_only_paths, while production now classifies it as a socket (56c8376). The ~3.7k lines of fixtures didn't catch the socket case and now encode behavior production doesn't have. Suggestion: inject the mount-table/path-kind lookups so tests drive the production function, and drop the hint path.
4. The resolver lives in openshell-core, which spreads the git dependency everywhere
Only openshell-sandbox calls resolve_cdi_context, validate_cdi_requirements, and CdiMountTable. The Docker driver needs only the CdiContext / CdiSpecDirectory schema. Placing the resolver in core pulls the git-pinned CDI crate (plus jsonschema, fancy-regex, oci-spec, derive_builder, …) into every Linux consumer of core. That's why examples/*/Cargo.lock each grow by ~550 lines. Keeping only the serde schema in core and moving the resolver into openshell-sandbox would contain it.
On the dependency itself: the git pin requires the deny.toml allow-git exception, the RPM vendoring change (4cad83e), and the serialize/deserialize workaround for crate-private edit fields. Is a crates.io release of container-device-interface-rs with those fields public feasible first? That would remove all three.
5. supplemental_groups duplicates the existing identity check (ProcessPolicy, SandboxPolicyWire, boundary_server.rs)
validate_running_identity / supplementary_groups_match already verify supplementary groups, including an allow-extra mode for GPU claims. The new ProcessPolicy.supplemental_groups is never applied, only compared against getgroups(). It could be a required-GID set passed to the existing check, with no change to ProcessPolicy or the wire format. As written it is also inconsistent: cdi_requirements is deliberately kept off the wire because it's derived in the workload, but supplemental_groups, derived the same way, is added to SandboxPolicyWire. (Minor: "supplemental" vs the existing "supplementary" naming.)
6. /sys docs don't match the code, and the grant is new (default-policy.mdx, apply_cdi_requirements)
The docs place the read-only /sys grant in the paragraph about GPU sandboxes without CDI context, but only the CDI path adds /sys. Legacy enrichment doesn't, and main doesn't grant /sys today. Since a read-only grant on all of /sys is a new permission, it would help to state what in CUDA init needs it and whether a narrower set works.
Smaller items
FilesystemPolicy.cdi_requirementsis runtime-local provenance on a struct shared with the supervisor, OPA, and all drivers, which is why seven unrelated files now setcdi_requirements: None. A sandbox-local launch context would keep it contained.- The broad-root list (
/,/dev,/proc,/sys,/run,/usr) is hard-coded incdi_linux.rs,cdi_paths.rs, and three doc pages. A single shared constant would help. - Library-parent widening still grants read-only access to directories like
/etc,/home, or/rootif a spec places a.sothere. Specs are host-admin trusted, so this is defense in depth, but please state the trust model. - With the descriptor-based pass in place, the path-based type checks in core's
validate_cdi_requirements(std::fs::metadata, follows symlinks) look redundant. Only the writable-file allowlist check still does work there. resolve_cdi_contextbuilds requirements with every mountread_only: false, callsmount_access()only for validation and discards the result, then rebuilds everything viaRequirementAccumulatorinclassify_cdi_mounts. A single pass would be easier to follow.- Socket validation is a type sanity check rather than enforcement: Landlock FS rules don't gate
connect(), as the new test shows. Worth wording the docs that way. - 56c8376 and d87c6ab are fixups to earlier commits and could be squashed.
- The
default-policy.mdxcorrection of the legacy/procwording is a pre-existing docs-drift fix and could land independently.
Optional: a behavior-oriented split
Not a blocker, just a suggestion in case it makes iteration easier. Comparing the fixture baselines against the default policy (/usr, /lib, /etc already readable), most of the user-visible value comes from device nodes and groups. Orin needs /dev/nvmap, /dev/nvhost-*, /dev/nvgpu/igpu0/*, /dev/host1x-fence, /dev/dri/*, and GIDs [44, 104, 995]. Spark needs /dev/dri/* and GIDs [44, 993]. None of these are in the legacy baseline. Nearly all derived read-only paths are already covered, so the mount-access machinery mostly adds new fail-closed checks for default policies.
Each PR below would land a behavior change:
- CDI device-node access for Docker GPU sandboxes. Schema, dependency, Docker projection/context/admission, and the sandbox resolving
deviceNodeswith descriptor validation. Unblocks Tegra/Spark GPU sandboxes; the legacy read-only baseline stays. - Require runtime-supplied CDI
additionalGidsthrough the existing identity check: fail fast with a clear error instead of inside CUDA. A natural place to work out why the runtime isn't supplying them (rsliter's IGX Orin report). - CDI-derived mount access. Effective-access classification, library-parent widening, writable-file allowlist, sockets,
/sys. This is where the fixtures belong. With the default policy it's mostly restrictive, so a concrete case it unblocks would help justify it. - Landlock
HardRequirementfor CDI sandboxes. Isolates the policy decision in issue 1.
2 and 3 build on 1 and are independent of each other. If fewer PRs is preferred, folding 4 (scoped to CDI rules) into 1 would also work.
Compatibility review (advisory)Unaddressed breaking changes Existing Docker GPU sandboxes can fail after upgrade because old containers lack newly required CDI projections. New GPU sandbox creation and policy enforcement also reject previously accepted host and policy configurations. 1. state — NOT ACKEDDocker GPU containers created before this change have no CDI projection mounts, but restart admission now requires every expected projection.
2. configuration — NOT ACKEDDocker GPU creation now bind-mounts every daemon-reported CDI spec directory and requires each source to exist and be readable.
3. policy — NOT ACKEDA writable CDI mount previously inherited an authored ancestor grant, but now it must be a regular file named by an exact
Review details
Advisory review; publication gates are unchanged. |
Compatibility review (advisory)Unaddressed breaking changes Existing Docker GPU sandboxes can fail after upgrade, and new GPU creation rejects previously accepted daemon-directory and filesystem-policy configurations. The PR documents the stricter rules but does not preserve compatibility for this non-breaking train. 1. state — NOT ACKEDBaseline GPU containers have only a CDI device request, while candidate restart admission requires CDI projection mounts that those persisted containers lack.
2. configuration — NOT ACKEDBaseline treats nonempty Docker
3. policy — NOT ACKEDA writable CDI mount previously inherited an authored ancestor
Review details
Advisory review; publication gates are unchanged. |
Compatibility review (advisory)Unaddressed breaking changes Three intentional Docker GPU incompatibilities remain in persisted sandbox state, daemon CDI-directory configuration, and filesystem policy behavior. The PR author explicitly acknowledges and documents them, but this train does not allow Stable breaks. 1. state — ACKEDBaseline Docker GPU containers have only a CDI device request, while candidate
2. configuration — ACKEDBaseline treats any nonempty Docker
3. policy — ACKEDA writable CDI destination previously inherited an authored ancestor
Review details
Advisory review; publication gates are unchanged. |
1d218a5 to
9a7c9b5
Compare
Compatibility review (advisory)Needs review Docker GPU launches now require every CDI 1. api-behavior — NOT ACKED
Review details
Advisory review; publication gates are unchanged. |
9a7c9b5 to
b724cc8
Compare
Compatibility review (advisory)Unaddressed breaking changes Docker GPU workloads using writable ordinary CDI mounts now fail even when policy authorizes the destination. Runtime compatibility for CDI 1. policy — ACKEDDocker GPU process launch previously accepted runtime-provided CDI mounts under authored
2. api-behavior — ACKEDDocker GPU launch and every later exec now require all CDI
Review details
Advisory review; publication gates are unchanged. |
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
1b40211 to
d365bb1
Compare
|
/ok-to-test d365bb1 |
Compatibility review (advisory)Unaddressed breaking changes Docker GPU sandboxes gain new CDI projection and policy requirements that can reject previously accepted launches. Candidate-created GPU state is also unreadable after downgrade, and the supplied no-break train does not permit these changes. 1. api-behavior — ACKEDDocker GPU creation previously submitted CDI device IDs directly, but now fails unless at least one daemon-reported CDI spec directory can be probed and projected into the workload.
2. policy — ACKEDOrdinary writable CDI file or directory mounts now fail launch even when
3. policy — ACKEDA CDI device node explicitly listed in
4. policy — NOT ACKEDCDI filesystem targets now override
5. state — ACKEDNew Docker GPU bootstraps persist
Review details
Advisory review; publication gates are unchanged. |
e9fa277 to
fb39ac1
Compare
Define the shared CDI context schema: the selected CDI device IDs and the workload-local paths at which a compute driver projects CDI spec directories. Carry it as an optional field in the sandbox boundary configuration, omitted from serialized output when absent so non-GPU bootstraps stay readable by older sandbox runtimes. No driver sets it yet. Signed-off-by: Evan Lezar <elezar@nvidia.com>
Resolve the selected CDI devices against the spec directories projected into the workload and derive the filesystem requirements their edits imply: - device nodes, which need read-write access; - read-only mounts, expanded to the parent directory for shared libraries; - Unix sockets, kept as exact targets without filesystem grants; - the NVIDIA MPS pipe directory, accepted as a writable IPC directory only when mounted nosuid, nodev and noexec; - additional GIDs, excluding root. Effective mount access comes from the workload's mount table rather than spec options. Ordinary writable mounts, broad roots and conflicting uses of one path are rejected. Captured Orin, Spark and multi-GPU specs drive the production resolver against reviewed baselines. The resolver is wired into launch in the next commit. Signed-off-by: Evan Lezar <elezar@nvidia.com>
When the boundary configuration carries a CDI context, resolve it at agent start and use the result instead of the hard-coded GPU baseline: - add derived read-only paths, and read-write access for device nodes and IPC directories, unless the authored policy already covers them; - reject an authored read-only entry for a device node or IPC directory instead of overriding it; - validate every CDI target through the descriptor that anchors its Landlock rule at each launch and exec, rejecting broad-root aliases, type changes and changed mount access; - require Landlock enforcement for CDI targets even when the policy is best effort. Missing CDI additionalGids are reported as a warning, because some runtimes do not apply them and devices that do not depend on them stay usable. A required root group remains fatal. Enrichment, failures and the effective Landlock mode are reported as OCSF events. Sandboxes without a CDI context keep the existing GPU baseline. Signed-off-by: Evan Lezar <elezar@nvidia.com>
For GPU sandboxes, record the selected CDI device IDs and the spec directories the Docker daemon currently reports in the protected boundary configuration, and project those directories read-only under /.openshell/cdi-specs so the sandbox runtime can resolve the same specs. User mounts, image volumes and the workspace cannot overlap the projection path. Before creating the sandbox, probe each directory with a stopped container from the prepared workload image and skip directories that do not exist on the Docker host. Probes are bounded by a timeout, labelled for cleanup with their sandbox, and remove their anonymous volumes. A probe that cannot be removed no longer fails creation. With resource admission enabled, containers with a CDI context must keep a CDI device request matching the recorded selection and one read-only, non-recursive, private bind per recorded directory. Admission reads the bootstrap only for CDI containers, and boundary files are replaced atomically so it never observes a partial write. The Docker e2e gateway enables resource admission in GPU mode. Signed-off-by: Evan Lezar <elezar@nvidia.com>
Describe how Docker GPU sandboxes derive filesystem access from the selected CDI devices, which CDI mounts are rejected, the NVIDIA MPS pipe directory exception, missing additionalGids handling on Docker releases before 29.5, and SELinux requirements for the projected spec directories. Add cluster debugging entries for CDI projection, admission, probe and restart failures, and document the CDI context in the core, sandbox and Docker driver READMEs. Signed-off-by: Evan Lezar <elezar@nvidia.com>
Record the behavior of the named scenarios once filesystem access is derived from the selected CDI specs: read-only mounts and library parents are readable, device nodes are writable, and writable mounts, an unsafe library parent, and an NVIDIA MPS directory without noexec are rejected. Add scenarios for the MPS pipe directory with and without noexec. Signed-off-by: Evan Lezar <elezar@nvidia.com>
d365bb1 to
33350a8
Compare
|
/ok-to-test 33350a8 |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. api-behavior — No explicit risk acceptance foundExplicit Docker
2. policy — No explicit risk acceptance foundDocker CDI paths absent from
3. policy — No explicit risk acceptance foundOrdinary writable CDI mounts changed from policy-controlled launches to unconditional launch rejection.
4. policy — No explicit risk acceptance foundA CDI device node in
5. state — No explicit risk acceptance foundCandidate-created Docker GPU bootstraps add
Review details
Advisory review; publication gates are unchanged. |
This branch has not been deployed
4 participants
Summary
Derive Docker GPU workload access from the selected CDI specifications, so Orin and other GPU systems receive the device-node, library, socket and group access that the fixed GPU allowlist does not cover.
Stacked on #4461 (
test/4460-tmachine-cdi/elezar), which is stacked on #4529 (codex/cdi-name-parser). The tmachine harness and named CDI scenario fixtures come from #4461. The upstream CDI revision, Git source allowlist and RPM vendoring belong to those prerequisites; this PR only adds the sandbox's references to existing dependencies.Related Issue
Related to #1606 (currently closed). Supersedes #2775, #2776, and #2265. The standard-sbin PATH repair remains a separate follow-up in #2846.
Changes
The six commits are ordered for review, and each builds and passes its tests on its own:
feat(core): CDI context in the boundary protocol. A versioned CDI context schema (selected device IDs and projected spec directories) carried as an optional boundary configuration field, omitted when absent. No driver sets it yet.feat(sandbox): resolve CDI requirements. Resolves the selected devices against the projected specs and derives device nodes, read-only paths (shared libraries expand to their parent directory), exact socket targets, additional GIDs, and the NVIDIA MPS pipe directory as a writable IPC directory when mountednosuid,nodev,noexec. Effective mount access comes from the workload's mount table. Ordinary writable mounts, broad roots and conflicting uses of one path are rejected. Captured Orin, Spark and multi-GPU specs run through the production resolver against reviewed baselines. Not yet wired in, so this commit has dead-code warnings.feat(sandbox): enforce CDI-derived access. At agent start, adds the derived paths unless authored policy already covers them, validates every CDI target through the descriptor that anchors its Landlock rule at each launch and exec, and requires Landlock enforcement for CDI targets even in best-effort mode. An authored read-only entry for a device node or IPC directory fails launch instead of being overridden. MissingadditionalGidsproduce a warning, not a failure. Enrichment, failures and the effective Landlock mode are reported as OCSF events. Sandboxes without a CDI context keep the existing GPU baseline.feat(docker): project spec directories and record the context. Records the selected devices and the spec directories the daemon currently reports, and projects them read-only, non-recursive and private under/.openshell/cdi-specs, which user mounts, image volumes and the workspace cannot overlap. Before creation, each directory is probed with a stopped container from the prepared image, bounded by a timeout and labelled for cleanup with its sandbox. Missing directories are skipped. With resource admission enabled, CDI containers must keep a device request matching the recorded selection and the recorded projections. Admission reads the bootstrap only for CDI containers, and boundary files are replaced atomically. The Docker e2e gateway enables resource admission in GPU mode.docs(gpu). Published policy and runtime docs, the cluster debugging skill and crate READMEs.test(cdi). A tmachine test that runs thedevice-node-with-gidfixture as non-root and requires the CDI group and read-write device access, plus MPS pipe directory fixtures (positive, and negative withoutnoexec).The NVIDIA Container Toolkit currently never emits the MPS pipe directory because its lookup rejects directories; NVIDIA/nvidia-container-toolkit#2143 fixes that.
Testing
aarch64-unknown-linux-gnu, non-root, Landlock required): 310 pass.network_broker::tests::broker_refuses_socket_families_it_cannot_confinefails in the local container both with and without this change.aarch64-unknown-linux-gnufor core, the Docker driver, the sandbox and the CDI feature-test crate;cargo fmt --checkpasses./dev/shmis writable after enforcement while sibling files stay denied, and the test fails when the directory is only read-only.additionalGidsare ignored by 29.4.3 and applied by 29.5.3.HostConfig.MountsreportsReadOnlyandNonRecursiveas configured.additionalGidsand fails without.mise run pre-commitnot run on this revision (the local compiler-cache wrapper fails in this environment); its Rust format and lint steps were run directly.Compatibility requirements
I accept the following compatibility changes for Docker GPU sandboxes:
read_writegrant names its destination or an ancestor. The only exception is the NVIDIA MPS pipe directory (/tmp/nvidia-mps) mountednosuid,nodev,noexec. Explicit authorization for CDI host writes is not supported. Device nodes and Unix sockets are unaffected.filesystem_policy.read_onlystops the launch instead of leaving the GPU unusable./.openshell/cdi-specsis reserved. User mounts, image volumes and an OCI working directory that overlap it are rejected for all Docker sandboxes.additionalGids. The workload still starts, an OCSF warning names the missing groups, and devices restricted to those groups may be inaccessible until Docker is upgraded and the sandbox restarted. This replaces the earlier launch failure./var/run/cdiafter a reboot without CDI spec regeneration. Regenerating the specs and starting again recovers it.Checklist
🤖 Generated with Claude Code