fix(sandbox): skip read-only mounts during recursive chown of /sandbox#2341
Conversation
The sandbox supervisor crashed with EROFS when the recursive chown of /sandbox encountered read-only submounts. This is common in gVisor-based Kubernetes deployments where read-only volume mounts are the only way to enforce per-directory immutability (Landlock is unavailable under gVisor). The fix adds two guards to the ownership walk: 1. Mount-boundary detection via st_dev comparison — paths on a different filesystem than /sandbox are skipped entirely, avoiding the chown call on nested read-only mounts. 2. EROFS tolerance — if chown still returns EROFS (e.g. the root mount itself is read-only), the error is logged at debug level and startup continues. Symlink skipping (already present) is preserved and extracted into the recursive walker for consistency. Manually verified on a kind cluster with a read-only PVC mounted at /sandbox/readonly-data: the pod starts successfully, writable paths are owned by the sandbox user, and the read-only mount retains root ownership. Kubernetes e2e test coverage is a separate follow-up. Closes NVIDIA#2294 Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
6dadfae to
90fae13
Compare
Move the per-child symlink guard from the directory iteration loop into the top of chown_recursive, reusing the symlink_metadata call that is already performed there. This centralizes all three skip guards (symlink, mount boundary, EROFS) in one place and eliminates a redundant symlink_metadata call per child entry. Suggested-by: elezar Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
When chown returns EROFS on a directory, the code previously returned early without recursing into children. This skipped writable submounts nested under a read-only directory on the same device (e.g., a writable emptyDir inside a read-only ConfigMap mount sharing the same st_dev). Now the EROFS case falls through to the directory recursion so that writable children are still chowned. Also fixes the doc comment which said EROFS errors are "logged as warnings" when they are actually logged at debug level. Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
|
Thank you @varshaprasad96 i did some validation testing and this MR does indeed fix the bug. Looking forward to it getting merged :) |
|
/ok-to-test 3d079d5 |
Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This is a concentrated fix for the confirmed Kubernetes sandbox startup bug in #2294.
Head SHA: 888f91a47dfd8f14f86986356a385b24cd805184
Review findings:
- Two actionable findings are attached inline: a writable cross-filesystem mount regression and missing coverage for the actual
EROFSrecovery path.
Thanks @elezar. I checked your request to centralize the symlink check; the current head resolves it in chown_recursive and preserves the no-follow behavior.
Docs: Fern docs are not required because the intended fix does not change a user-facing command, configuration, or workflow.
Next state: gator:in-review; author changes are required before pipeline monitoring.
johntmyers
added
the
gator:in-review
Remove the unconditional st_dev boundary check that prevented chown on writable PVC mounts with a different device ID. Rely solely on EROFS errors to skip read-only paths while continuing traversal into children and siblings. Inject the chown operation into chown_recursive to enable unit testing the EROFS recovery path without requiring root or mount privileges. Signed-off-by: Varsha Prasad Narsing <vnarsing@nvidia.com> Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
|
Label |
|
/ok to test 0869591 |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This is a concentrated fix for the confirmed Kubernetes sandbox startup bug in #2294.
Head SHA: 08695910c048edf59643e547bef38b1af61bf38a
Review findings:
- No blocking findings remain.
- The independent review found only a non-blocking follow-up suggestion to add Kubernetes E2E coverage for a real read-only submount.
Thanks @varshaprasad96. I verified that the current update resolves both prior gator findings: it removes the cross-device skip that could leave writable PVCs root-owned, and the injected chown test now exercises EROFS recovery while confirming traversal continues. Non-EROFS failures still propagate, and symlinks remain skipped.
Docs: Fern docs are not required because this restores startup behavior without changing a command, configuration field, API, or documented workflow.
E2E: test:e2e is applied for the sandbox-lifecycle change. The current-head mirror refresh was authorized with /ok to test 08695910c048edf59643e547bef38b1af61bf38a; required checks should run after the mirror catches up.
Next state: gator:watch-pipeline.
johntmyers
added
gator:watch-pipeline
Signed-off-by: Varsha Prasad Narsing <vnarsing@nvidia.com> Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains a concentrated fix for the confirmed Kubernetes sandbox startup bug in #2294.
Head SHA: 2250cb23c012f9da545b81bfeb9d0b099406e115
Review findings:
- One blocking high-severity security finding is attached inline: the root-privileged path-based ownership walk remains vulnerable to a symlink swap on concurrently mutable mounted content.
- A real Kubernetes read-only-mount regression test remains a useful non-blocking follow-up; the injected unit test cannot exercise actual mount, gVisor, or LSM behavior.
Docs: Fern docs are not required because this restores behavior already promised for read-only PVC configuration and adds no command, option, API, or workflow.
Next state: gator:in-review; author changes are required before pipeline monitoring resumes.
johntmyers
added
gator:in-review
|
/ok to test 2250cb2 |
After chown returns EROFS, return immediately instead of recursing into the read-only directory's children. Descending into a mount we cannot own creates a TOCTOU window where an attacker could swap a child for a symlink between symlink_metadata and chown (CWE-367/CWE-59). Siblings of the EROFS path are still visited by the caller's loop. Signed-off-by: Varsha Prasad Narsing <vnarsing@nvidia.com> Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
…ate double stat Chown root directly in chown_sandbox_home so EROFS on /sandbox itself is a hard startup error rather than being silently swallowed. Extract chown_children to log read_dir failures instead of silently skipping them. This also eliminates the redundant symlink_metadata syscall on the root path. Signed-off-by: Varsha Prasad Narsing <vnarsing@nvidia.com> Signed-off-by: Varsha Prasad Narsing <varshaprasad96@gmail.com>
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains a concentrated fix for the confirmed Kubernetes sandbox startup bug in #2294.
Head SHA: 9d6e49df2bc39831147ad02a3618bc2e2ec9bf3e
Review findings:
- No blocking findings remain.
- The independent review confirmed that child-level
EROFSnow stops only that subtree, writable siblings and writable cross-filesystem mounts remain supported, and non-EROFSfailures still propagate. - A real Kubernetes/gVisor read-only-mount regression test remains a useful non-blocking follow-up; the injected unit tests validate the branching but not the production mount behavior.
Thanks @varshaprasad96. I checked your update addressing the prior symlink-swap concern: the current head returns immediately after EROFS, before chown_children, so it no longer descends into the unowned subtree while the caller continues with siblings.
Docs: Fern docs are not required because this restores the documented read-only PVC behavior without changing a command, configuration field, API, or workflow.
E2E: test:e2e is already applied for the sandbox-lifecycle change. The copy mirror is stale and needs current-head authorization before required checks can run.
Next state: gator:watch-pipeline.
johntmyers
added
gator:watch-pipeline
|
/ok to test 9d6e49d |
johntmyers
added
gator:merge-ready
and removed
gator:watch-pipeline
|
@johntmyers can we add this to the merge queue again. Looks like the error in merge queue was related to not being able to fetch image: |
Monitoring CompleteMonitoring is complete because this PR has merged. Final status: The PR reached I removed the active |
4 participants
Summary
Fix sandbox supervisor crash (
EROFS: Read-only file system) when the recursivechown /sandboxencounters read-only submounts. This is common in gVisor-based Kubernetes deployments where read-only volume mounts enforce per-directory immutability (Landlock is unavailable under gVisor).Related Issue
Closes #2294
Changes
st_devof each path against the root/sandboxdevice — paths on a different filesystem are skipped entirely, avoiding thechowncall on nested read-only mountschownstill returnsEROFS(e.g. the root mount itself is read-only), the error is logged at debug level and startup continueschown_sandbox_home: Split into entry-point (symlink guard + root_dev capture) andchown_recursive(inner walk with st_dev and EROFS guards)chown_recursive_skips_different_deviceandchown_sandbox_home_does_not_chown_across_device_boundary; strengthened existing symlink rejection testTesting
mise run pre-commitpasses (Rust lint/format/clippy clean; unrelated Python proto env issue)/sandbox/readonly-data: pod starts successfully, writable paths owned by sandbox user, read-only mount retains root ownershipChecklist