Skip to content

Allow for raw marshalling of BackingIds#660

Open
Popax21 wants to merge 4 commits intocberner:masterfrom
Popax21:master
Open

Allow for raw marshalling of BackingIds#660
Popax21 wants to merge 4 commits intocberner:masterfrom
Popax21:master

Conversation

@Popax21
Copy link

@Popax21 Popax21 commented Feb 25, 2026

Adds methods to convert BackingIds from/to their raw backing_id values, which allows for marshalling of backing file references across process boundaries.

Currently, the Linux kernel still requires CAP_SYS_ADMIN to 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 BackingId reference across process boundaries; this PR adds two methods, BackingId::into_raw() and ReplyOpen::wrap_backing(), to convert the safe Rust wrapper to/from the underlying raw backing_id value 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 the BackingId erroring out.
However just to be safe / clearly communicate the "raw-ness" of the operation I have still decided to mark it as unsafe anyway.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Copy link
Owner

@cberner cberner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Popax21
Copy link
Author

Popax21 commented Feb 27, 2026

Please add a test for this feature, that tests it works in a multi process setup like you described.

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).

Also, I don't think unsafe is appropriate here. Those methods don't look like they can cause memory safety issues.

As explained in the PR description this is mostly a precautionary measure, following the precedent set by std::os::fd::FromRawFd / Rust's IO safety concept. However, I can remove the unsafe specifier if you still think that doing so would be the right call.

@Popax21
Copy link
Author

Popax21 commented Feb 28, 2026

Fixed the rustc lint build failure (seems like it only appears on nightly, and I ran my pre-commit checks on stable; oops...)

@Popax21
Copy link
Author

Popax21 commented Mar 1, 2026

Ok, lints are just sending us in circles now; seems like we either get a unused-parens lint, or a break-with-label-and-loop lint. Either rust-lang/rust#137414 regressed, or the CI's rustc doesn't include the fix for that issue yet. Should I try to refactor the code to avoid the snippet in question, or should I just #[allow(...)] one of the lints?

@cberner
Copy link
Owner

cberner commented Mar 1, 2026

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

@cberner
Copy link
Owner

cberner commented Mar 1, 2026

I merged #661

Can you try rebasing on master?

@Popax21
Copy link
Author

Popax21 commented Mar 1, 2026

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.

Copy link
Owner

@cberner cberner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please submit this in a separate PR, since it isn't related to the BackingId feature, as far as I can tell

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Popax21 added 3 commits March 3, 2026 02:27
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.
@Popax21
Copy link
Author

Popax21 commented Mar 3, 2026

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

Done; the passthrough_fork.rs example now runs as part of CI, similarly to how passthrough.rs already does (I was initially under the assumption that the CI runner does not have access to CAP_SYS_ADMIN, so I did not implement this at first; sorry for the mistake). Also addressed the other PR reviews.

@cberner
Copy link
Owner

cberner commented Mar 5, 2026

CI is failing. It looks like your rebase on master didn't work correctly, because it's trying to use nightly

@Popax21
Copy link
Author

Popax21 commented Mar 5, 2026

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.

@Popax21
Copy link
Author

Popax21 commented Mar 6, 2026

CI failure seems to be unrelated to this PR, stemming from examples/hello.rs which is not changed in any way by the PR (the non-Mac CI seems to have passed successfully as well). Not quite sure what would cause this; maybe the MacStadium CI run was flaky?

@cberner
Copy link
Owner

cberner commented Mar 7, 2026

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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)) })

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

Copy link
Owner

@cberner cberner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix the serialization bug Codex found. Otherwise lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants