Skip to content

kernel::fs::file::LocalFile should be marked as repr(transparent) #1165

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pekkarr opened this issue May 25, 2025 · 4 comments
Open

kernel::fs::file::LocalFile should be marked as repr(transparent) #1165

pekkarr opened this issue May 25, 2025 · 4 comments
Labels
good first issue Good for newcomers • lib Related to the `rust/` library. medium Expected to be an issue of medium difficulty to resolve.

Comments

@pekkarr
Copy link

pekkarr commented May 25, 2025

Unlike File which uses repr(transparent), LocalFile uses the implicit repr(Rust) representation which has an unspecified layout. This can break unsafe code that assumes that it has the same layout as bindings::file.

For example, the LocalFile::from_raw_file function casts *const bindings::file to *const LocalFile:

pub unsafe fn from_raw_file<'a>(ptr: *const bindings::file) -> &'a LocalFile {
// SAFETY: The caller guarantees that the pointer is not dangling and stays valid for the
// duration of 'a. The cast is okay because `File` is `repr(transparent)`.
//
// INVARIANT: The caller guarantees that there are no problematic `fdget_pos` calls.
unsafe { &*ptr.cast() }
}

Note that the safety comment here should be talking about LocalFile instead of File (probably a copy-paste error). Some other comments also refer to the wrong type (lines 227 and 238).

In practice, the default representation most likely uses the same layout as with repr(transparent), but it's best to not rely on that since it's unspecified.

@ojeda
Copy link
Member

ojeda commented May 25, 2025

Sounds good to me.

Cc: @Darksonn @y86-dev

@ojeda ojeda added • lib Related to the `rust/` library. good first issue Good for newcomers medium Expected to be an issue of medium difficulty to resolve. labels May 25, 2025
@y86-dev
Copy link
Member

y86-dev commented May 25, 2025

yup this should be changed to be repr(transparent) and also update the safety comments. @pekkarr do you want to take a shot at this?

@pekkarr
Copy link
Author

pekkarr commented May 25, 2025

Sure! I'll send a patch to the mailing list.

@ojeda
Copy link
Member

ojeda commented May 25, 2025

Thanks!

intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue May 27, 2025
Unsafe code in `LocalFile`'s methods assumes that the type has the same
layout as the inner `bindings::file`. This is not guaranteed by the default
struct representation in Rust, but requires specifying the `transparent`
representation.

The `File` struct (which also wraps `bindings::file`) is already marked as
`repr(transparent)`, so this change makes their layouts equivalent.

Fixes: 8518498 ("rust: file: add Rust abstraction for `struct file`")
Closes: Rust-for-Linux#1165
Signed-off-by: Pekka Ristola <[email protected]>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this issue May 27, 2025
Some of the safety comments in `LocalFile`'s methods incorrectly refer to
the `File` type instead of `LocalFile`, so fix them to use the correct
type.

Also add missing Markdown code spans around lifetimes in the safety
comments, i.e. change 'a to `'a`.

Link: Rust-for-Linux#1165
Signed-off-by: Pekka Ristola <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers • lib Related to the `rust/` library. medium Expected to be an issue of medium difficulty to resolve.
Development

No branches or pull requests

3 participants