fix(fuse): enforce read-only flags for direct mounts#742
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughRead-only FUSE mounts now set platform-specific direct mount flags. Linux combines ChangesRead-only mount option handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de222e6c16
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| if opts.ReadOnly { | ||
| fuseOpts.Options = append(fuseOpts.Options, "ro") | ||
| fuseOpts.DirectMountFlags = readOnlyDirectMountFlags() |
There was a problem hiding this comment.
Translate ro instead of forwarding it to direct mount
When --read-only is combined with --direct-mount-strict on Linux, this still leaves ro in fuseOpts.Options. In the pinned go-fuse fork, mountDirect only translates nodev/dev/nosuid/suid/noexec/exec from Options; other strings are passed into the FUSE data string, and its own comment says unrecognized data makes syscall.Mount fail with EINVAL. As a result the new MS_RDONLY flag does not make direct read-only mounts work unless ro is omitted or translated for the direct-mount path.
Useful? React with 👍 / 👎.
Summary
MS_RDONLYfor Linux read-only direct mountsMS_NODEV | MS_NOSUIDflags when overridingDirectMountFlagsromount option for the fusermount pathProblem
Drive9 maps
--read-onlyto the FUSE option stringro. This works through fusermount, which consumes-o ro, but the pinned go-fuse direct-mount path does not translaterointo theMS_RDONLYmount flag.As a result, a read-only direct mount could be mounted successfully while the kernel still reported it as writable. Drive9's userspace handlers reject mutating operations, but the mount itself was not protected by the kernel's read-only boundary.
Fix
For Linux read-only mounts, set:
DirectMountFlagsreplaces go-fuse's defaults when nonzero, soMS_NODEVandMS_NOSUIDmust be retained explicitly. The existingrooption remains in place so the fusermount path continues to work unchanged.The mount flags are implemented behind Linux/non-Linux build-tagged helpers so non-Linux builds do not reference Linux-only
syscall.MS_*constants.Validation
DirectMountFlags = 0x0, want 0x7go test ./pkg/fuse -run 'Test(ReadOnlyDirectMountFlags|GoFuseMountOptionsMaps(ReadOnly|DirectMountStrict))' -count=1go test ./pkg/fuse -count=1pkg/fusetest cross-compilepkg/fusetest cross-compilemake lintmake buildgit diff --checkThe repository-wide test run reached all non-database packages successfully, but database-backed packages could not start because the local environment does not have permission to access the Docker socket used by testcontainers.
Privileged runtime acceptance
A Linux environment with direct-mount privileges should additionally verify a
--read-only --direct-mount-strictmount with:findmntreportsrostatvfsincludesST_RDONLYEROFSSummary by CodeRabbit
Bug Fixes
Tests