Let the simple example be mounted idmapped - #750
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d03c5612e5
ℹ️ 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".
| self.idmapped = config.add_capabilities(InitFlags::FUSE_ALLOW_IDMAP).is_ok(); | ||
| if self.idmapped { | ||
| info!("idmapped mounts enabled; the kernel makes the access checks"); | ||
| } else { | ||
| warn!("idmapped mounts unavailable on this mount"); |
There was a problem hiding this comment.
When --idmap is used on a kernel that does not advertise FUSE_ALLOW_IDMAP (or any other case where add_capabilities refuses it), this branch only logs a warning and continues, but main has already mounted with DefaultPermissions and SessionACL::All. That silently turns a failed idmap request into a normal allow_other mount visible to other local users, which is broader access than the user asked for; returning an init error here would avoid exposing the mount in the fallback case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in 8738966. init() now returns ErrorKind::Unsupported rather than warning and serving on.
The exposure is exactly as described: allow_other is not something --idmap asks for on its own merits, it is the price the capability charges, and continuing without the capability leaves the price paid for nothing - a mount every local user can reach.
Verified both paths rather than only the one this kernel takes. On a kernel that does offer it, the mount comes up and serves requests as before. For the refusal, I withheld default_permissions in main to force add_capabilities to refuse, which is the same branch an older kernel reaches:
error reported: does not offer FUSE_ALLOW_IDMAP
mount left behind? 0
mountpoint clean
So the mount is torn down rather than left dangling, which was the part worth checking - an init error arrives after the mount is already up.
One thing I left alone: the process still exits 0, because main only maps PermissionDenied to a non-zero status and every other mount failure logs and returns. That is the example's existing behavior for all mount errors rather than anything this adds, and changing it would change the exit status for every failure mode at once, including for the xfstests harness. Worth doing, but not quietly here.
Generated by Claude Code
d03c561 to
8738966
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87389661f3
ℹ️ 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".
| (owner.uid != crate::FUSE_INVALID_UIDGID && owner.gid != crate::FUSE_INVALID_UIDGID) | ||
| .then_some(owner) |
There was a problem hiding this comment.
For a normal rename on a non-idmapped mount, the request header still has the caller's uid/gid, so this helper returns Some(owner) even when RENAME_WHITEOUT is not set. The new Filesystem::rename argument is documented as the owner for the inode created by a whiteout; exposing Some for every ordinary rename makes downstream implementations that use owner.is_some() to detect or validate whiteout creation take the wrong path. Pass None unless the rename flags actually include RENAME_WHITEOUT, and Some only for that case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and fixed in bce2947. The flags decide it now, so Some means the rename creates an inode, on an idmapped mount or otherwise.
I had made the Option mean "the kernel sent ids" and then documented my way around the gap - the doc says it is Some whatever the flags say off an idmapped mount. Documenting a type that says the wrong thing is not the same as fixing it, and owner.is_some() is the obvious reading for a filesystem to take.
It is the same conflation the rest of #748 set out to remove: ids arriving is not the same fact as an inode being created, and the argument should carry the one that matters to the caller. Off an idmapped mount the two coincide for every request, which is exactly why the wrong one looked right.
Pinned by a test rather than left to the doc, the doc being what let it through. It renames twice over a live mount, once plainly and once with RENAME_WHITEOUT, and asserts which of the two was named an owner - on a non-idmapped mount, where the header carries ids for both and only the flags separate them.
This is library code rather than the example the rest of this PR is about, so it is its own commit; happy to land it separately if you would rather.
Generated by Claude Code
Adds --idmap, which requests FUSE_ALLOW_IDMAP and mounts with the two things that capability needs: default_permissions, without which the kernel refuses the connection, and allow_other, without which fuser refuses the capability. Off by default, for a reason worth stating: negotiating it withholds the caller's ids from every request, whether or not anyone has made an idmapped mount, and this filesystem's own permission and privilege logic is built on those ids. An idmapped mount has no caller for the filesystem to check, so every check has to come from somewhere else - and does, for access. default_permissions is what makes the capability available at all, so the kernel has already decided, against ids it can map and this filesystem cannot. The caller's ids reached 85 places in three shapes - a mode-bit check, a test for root, and a test for owning an inode - and each is now may_access(), caller_is_root() or caller_owns(), which answer "allowed" where there is no caller to ask about. Privileges are the other half, and they go the other way. default_permissions decides who may act, not whose setuid and setgid survive acting, so nothing has decided that on the filesystem's behalf; caller_keeps_privileges() and caller_outside_group() answer so that a bit which cannot be shown to have been earned is dropped. FUSE_HANDLE_KILLPRIV_V2 is not negotiated at all under --idmap: taking that job on means judging a caller, and not every operation which drops these bits carries a flag saying so. The kernel keeps it, knowing the mapping. Two things that were not permission checks had to keep their meaning. truncate() took a uid and gid only to check access, and one caller passed 0 to skip that check, since a file handle already granted write access carries it whatever the mode says now; that is an argument now rather than a sentinel. And has_fsetid() takes the uid only for when /proc cannot be read, so it takes an Option and an unknown caller falls back to nothing rather than to root. generic/644, 656, 689 and 698 pass with the flag. The suite does not set it: the withheld ids cost generic/193, 683, 684 and 685, which check what this filesystem does with suid and sgid, and enabling four tests by breaking four others is not a trade worth making. generic/645 and 699 would stay excluded either way - 645 creates thousands of files in one directory, which is quadratic here, and 699 wants overlayfs on top, as the already-excluded 631 does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q4hiZwHE9fEYdn7DK3ZrV2
`Option<Owner>` on rename meant "the kernel sent ids" rather than "there is an inode to own", and off an idmapped mount every request carries ids. So an ordinary rename, which creates nothing, was handed an owner all the same, and a filesystem reading `owner.is_some()` as "this is a whiteout" would have been told yes for every rename. The flags decide it now. A rename creates an inode only with RENAME_WHITEOUT, so that is the one that gets an owner, on an idmapped mount or otherwise. Pinned by a test that renames twice over a live mount, once plainly and once with the flag, and checks which of the two was named an owner - the case the documentation described but nothing held it to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q4hiZwHE9fEYdn7DK3ZrV2
bce2947 to
5eab41e
Compare
2 participants
Adds
--idmap, which requestsFUSE_ALLOW_IDMAPand mounts with the two thingsthat capability needs:
default_permissions, without which the kernel refusesthe connection, and
allow_other, without which fuser refuses the capability.Why it is a flag rather than the default
Negotiating it withholds the caller's ids from every request, whether or not
anyone has made an idmapped mount - and this filesystem's permission and privilege
logic is built on those ids. That has a measurable cost, below.
Access and privilege need opposite answers
This is the part worth reviewing. An idmapped mount has no caller to check, and
the two kinds of question that reached for one do not fall the same way:
Access - may the caller act?
default_permissionsis what makes thecapability available at all, so the kernel has already decided, against ids it
can map and this filesystem cannot. An unknown caller is allowed. The ids reached
85 places in three shapes - a mode-bit check, a test for root, a test for owning
an inode - now
may_access(),caller_is_root()andcaller_owns().Privilege - do setuid and setgid survive?
default_permissionsdecides whomay act, not whose privileges survive acting, so nothing has decided this on the
filesystem's behalf. An unknown caller keeps nothing:
caller_keeps_privileges()andcaller_outside_group()are deliberately theother way round, so a bit that cannot be shown to have been earned is dropped.
I had these the same way round at first, which retained setgid where it should
have gone - the unsafe direction. generic/683, 684 and 685 caught it.
FUSE_HANDLE_KILLPRIV_V2follows from the same reasoning and is not negotiatedunder
--idmapat all: taking that job on means judging a caller, and not everyoperation that drops these bits carries a flag saying so. The kernel keeps it,
knowing the mapping.
Two things that were not permission checks
truncate()took a uid and gid only to check access, and one caller passed0to skip the check - a file handle already granted write access carries it
whatever the mode says now. That is an argument now rather than a sentinel.
has_fsetid()takes the uid only for when/proccannot be read, so it takesan
Optionand an unknown caller falls back to nothing rather than to root.What it does and does not enable
generic/644, 656, 689 and 698 pass with the flag - measured, not assumed.
The suite does not set it, and
xfstests.shis unchanged. Enabling itsuite-wide costs generic/193, 683, 684 and 685, which check what this filesystem
does with suid and sgid and depend on the ids the capability withholds. Four
tests gained for four lost is not a trade worth making, and the four that break
are the more fundamental ones.
Two of the six would stay excluded either way, neither for anything to do with
idmapping:
name here reads the directory's whole content, so the test's own cost is
quadratic - it makes steady progress (16k requests in the first 30s, still
climbing at 55k after five minutes) and would finish, given long enough for
that to be worth doing.
ESTALE, which is what generic/631 is already excluded for: overlayfs asks forfile handles this filesystem does not hand out.
Testing
Full suite, green: 175 tests run, 0 failures, no filesystem panics - the run that
matters, since this rewrote every permission check in the example.
Behavior checked directly as well: chattr's immutable and append-only rules, the
tmpfile round trip, creation times, and setgid inheritance in a setgid directory
all still hold, and an unprivileged caller is still refused a 0700 directory and
still allowed a 0777 one.
All six of CI's gates clean locally - fmt, clippy at each of the three feature
sets
make preuses, the macOS check with--all-targets, rustdoc, and the testsuite.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Q4hiZwHE9fEYdn7DK3ZrV2
Generated by Claude Code