[PATCH 1/2] rust: file: mark `LocalFile` as `repr(transparent)`

[PATCH 1/2] rust: file: mark `LocalFile` as `repr(transparent)`

[Pekka Ristola]

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: 851849824bb5 ("rust: file: add Rust abstraction for `struct file`")
Closes: https://github.com/Rust-for-Linux/linux/issues/1165
Signed-off-by: Pekka Ristola <pekkarr@protonmail.com>
---
 rust/kernel/fs/file.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs
index 13a0e44cd1aa..138693bdeb3f 100644
--- a/rust/kernel/fs/file.rs
+++ b/rust/kernel/fs/file.rs
@@ -219,6 +219,7 @@ unsafe fn dec_ref(obj: ptr::NonNull<File>) {
 ///   must be on the same thread as this file.
 ///
 /// [`assume_no_fdget_pos`]: LocalFile::assume_no_fdget_pos
+#[repr(transparent)]
 pub struct LocalFile {
     inner: Opaque<bindings::file>,
 }
-- 
2.49.0

[PATCH 2/2] rust: file: improve safety comments

[Pekka Ristola]

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: https://github.com/Rust-for-Linux/linux/issues/1165
Signed-off-by: Pekka Ristola <pekkarr@protonmail.com>
---
 rust/kernel/fs/file.rs | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/fs/file.rs b/rust/kernel/fs/file.rs
index 138693bdeb3f..72d84fb0e266 100644
--- a/rust/kernel/fs/file.rs
+++ b/rust/kernel/fs/file.rs
@@ -225,7 +225,7 @@ pub struct LocalFile {
 }
 
 // SAFETY: The type invariants guarantee that `LocalFile` is always ref-counted. This implementation
-// makes `ARef<File>` own a normal refcount.
+// makes `ARef<LocalFile>` own a normal refcount.
 unsafe impl AlwaysRefCounted for LocalFile {
     #[inline]
     fn inc_ref(&self) {
@@ -236,7 +236,8 @@ fn inc_ref(&self) {
     #[inline]
     unsafe fn dec_ref(obj: ptr::NonNull<LocalFile>) {
         // SAFETY: To call this method, the caller passes us ownership of a normal refcount, so we
-        // may drop it. The cast is okay since `File` has the same representation as `struct file`.
+        // may drop it. The cast is okay since `LocalFile` has the same representation as
+        // `struct file`.
         unsafe { bindings::fput(obj.cast().as_ptr()) }
     }
 }
@@ -274,7 +275,7 @@ pub fn fget(fd: u32) -> Result<ARef<LocalFile>, BadFdError> {
     #[inline]
     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)`.
+        // duration of `'a`. The cast is okay because `LocalFile` is `repr(transparent)`.
         //
         // INVARIANT: The caller guarantees that there are no problematic `fdget_pos` calls.
         unsafe { &*ptr.cast() }
@@ -348,7 +349,7 @@ impl File {
     #[inline]
     pub unsafe fn from_raw_file<'a>(ptr: *const bindings::file) -> &'a File {
         // 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)`.
+        // 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() }
-- 
2.49.0

Re: [PATCH 2/2] rust: file: improve safety comments

[Alice Ryhl]

On Tue, May 27, 2025 at 08:48:59PM +0000, Pekka Ristola wrote:
> 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: https://github.com/Rust-for-Linux/linux/issues/1165
> Signed-off-by: Pekka Ristola <pekkarr@protonmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 2/2] rust: file: improve safety comments

[Benno Lossin]

On Tue May 27, 2025 at 10:48 PM CEST, Pekka Ristola wrote:
> 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: https://github.com/Rust-for-Linux/linux/issues/1165
> Signed-off-by: Pekka Ristola <pekkarr@protonmail.com>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  rust/kernel/fs/file.rs | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Re: [PATCH 1/2] rust: file: mark `LocalFile` as `repr(transparent)`

[Alice Ryhl]

On Tue, May 27, 2025 at 08:48:55PM +0000, Pekka Ristola wrote:
> 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: 851849824bb5 ("rust: file: add Rust abstraction for `struct file`")
> Closes: https://github.com/Rust-for-Linux/linux/issues/1165
> Signed-off-by: Pekka Ristola <pekkarr@protonmail.com>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH 1/2] rust: file: mark `LocalFile` as `repr(transparent)`

[Miguel Ojeda]

On Tue, May 27, 2025 at 10:49 PM Pekka Ristola <pekkarr@protonmail.com> wrote:
>
> 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: 851849824bb5 ("rust: file: add Rust abstraction for `struct file`")
> Closes: https://github.com/Rust-for-Linux/linux/issues/1165
> Signed-off-by: Pekka Ristola <pekkarr@protonmail.com>

Thanks Pekka, both patches look good to me. I will close the issue
when Christian applies them (or if I should take them, that is good
too).

Cheers,
Miguel

Re: [PATCH 1/2] rust: file: mark `LocalFile` as `repr(transparent)`

[Pekka Ristola]

On Wednesday, May 28th, 2025 at 13.52, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Tue, May 27, 2025 at 10:49 PM Pekka Ristola <pekkarr@protonmail.com> wrote:
> 
> > 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: 851849824bb5 ("rust: file: add Rust abstraction for `struct file`")
> > Closes: https://github.com/Rust-for-Linux/linux/issues/1165
> > Signed-off-by: Pekka Ristola pekkarr@protonmail.com
> 
> 
> Thanks Pekka, both patches look good to me. I will close the issue
> when Christian applies them (or if I should take them, that is good
> too).

Thanks, I'm glad it went smoothly since this was my first patch to the
mailing list.

Pekka

Re: [PATCH 1/2] rust: file: mark `LocalFile` as `repr(transparent)`

[Benno Lossin]

On Tue May 27, 2025 at 10:48 PM CEST, Pekka Ristola wrote:
> 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: 851849824bb5 ("rust: file: add Rust abstraction for `struct file`")
> Closes: https://github.com/Rust-for-Linux/linux/issues/1165
> Signed-off-by: Pekka Ristola <pekkarr@protonmail.com>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

Re: [PATCH 1/2] rust: file: mark `LocalFile` as `repr(transparent)`

[Christian Brauner]

On Tue, 27 May 2025 20:48:55 +0000, Pekka Ristola wrote:
> 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.
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/2] rust: file: mark `LocalFile` as `repr(transparent)`
      https://git.kernel.org/vfs/vfs/c/15ecd83dc062
[2/2] rust: file: improve safety comments
      https://git.kernel.org/vfs/vfs/c/946026ba4293