Allow for raw marshalling of BackingIds#660
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3e1c3b9f2
ℹ️ 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".
cberner
left a comment
There was a problem hiding this comment.
Please add a test for this feature, that tests it works in a multi process setup like you described.
Also, I don't think unsafe is appropriate here. Those methods don't look like they can cause memory safety issues.
Done; over the course of programming the example, I have encountered some other minor issues that I have decided to clean up as I went along (see the two respective comments for details).
As explained in the PR description this is mostly a precautionary measure, following the precedent set by |
|
Fixed the rustc lint build failure (seems like it only appears on nightly, and I ran my pre-commit checks on stable; oops...) |
|
Ok, lints are just sending us in circles now; seems like we either get a |
|
I wondered if we'd run into issues with nightly rustfmt regressions. I'll see if I can revert it to stable. I believe it's only needed for one format option, that's nice to have but not really required |
|
I merged #661 Can you try rebasing on master? |
Done; also removed the extra parenthesis again since hopefully that lint should no longer trigger a false positive. EDIT: since the lint still appeared I've just refactored the offending line away now. |
cberner
left a comment
There was a problem hiding this comment.
This feature still needs a test that runs as part of CI. It should test that it works as expected: backing ids can be passed from a privileged process to an unprivileged process and used there
| mountpoint: P, | ||
| options: &Config, | ||
| ) -> io::Result<()> { | ||
| check_option_conflicts(options)?; |
There was a problem hiding this comment.
Please submit this in a separate PR, since it isn't related to the BackingId feature, as far as I can tell
There was a problem hiding this comment.
This was mainly a drive-by change while implementing the other session fixes / improvements, since currently one can bypass this option conflict check by calling Session::new() directly. Moving this call there cleans up the module-level entrypoints while also patching this minor blind spot.
It is however true that this is unrelated to the actual purpose of the PR; as such I can factor out this commit (which also includes the run() visibility change) into a separate PR. My reasoning for not doing so initially was that this was a rather minor change, so the bureaucratic overhead of making a new PR for it instead of just doing this is a drive-by seemed unreasonable to me.
Adds methods to convert BackingIds from/to their raw `backing_id` values, which allows for marshalling of backing file references across process boundaries.
The kernel rejects all `FOPEN_PASSTHROUGH` replies for an inode that's already open in passthrough mode if the given `backing_id` does not match the current `backing_id`. Clarify this in the relevant method documentation.
Expose the the `run` method while fixing an options check blindspot.
Done; the |
|
CI is failing. It looks like your rebase on master didn't work correctly, because it's trying to use nightly |
Seems like some of the upstream Makefile changes weren't reapplied correctly. Should be fixed now. |
|
CI failure seems to be unrelated to this PR, stemming from |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de769b9208
ℹ️ 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".
| let mut buf = [0u8; 4]; | ||
| self.socket.send(FILE.as_bytes())?; | ||
| self.socket.recv(&mut buf)?; | ||
| Ok(unsafe { reply.wrap_backing(u32::from_ne_bytes(buf)) }) |
There was a problem hiding this comment.
Decode backing ID with matching byte order
This deserializes the ID with u32::from_ne_bytes, but the server side in the same example sends it with u32::to_le_bytes (examples/passthrough_fork.rs:333). On big-endian systems this will reinterpret the bytes incorrectly, so wrap_backing receives the wrong backing_id and passthrough opens can fail even though the parent created a valid ID. Use a matching decode (from_le_bytes) or switch both ends to native endianness.
Useful? React with 👍 / 👎.
cberner
left a comment
There was a problem hiding this comment.
Please fix the serialization bug Codex found. Otherwise lgtm
Adds methods to convert
BackingIdsfrom/to their rawbacking_idvalues, which allows for marshalling of backing file references across process boundaries.Currently, the Linux kernel still requires
CAP_SYS_ADMINto use FUSE passthrough.Since this is a highly privileged capability, FUSE filesystem drivers may wish to move this part of their FS into a privileged host process, while continuing to run the rest of the FS at a lower privilege level.
However, this requires the ability to move a
BackingIdreference across process boundaries; this PR adds two methods,BackingId::into_raw()andReplyOpen::wrap_backing(), to convert the safe Rust wrapper to/from the underlying rawbacking_idvalue respectively, which the user may then e.g. send across process boundaries.ReplyOpen::wrap_backing()is marked as unsafe, even though as far as I can tell misuse of this method does not result in anything else than subsequent operations on theBackingIderroring out.However just to be safe / clearly communicate the "raw-ness" of the operation I have still decided to mark it as unsafe anyway.