[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4bf5bf3b-88f4-4ee4-80fd-c566428d9f69@proton.me>
Date: Tue, 06 Aug 2024 08:44:02 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Alice Ryhl <aliceryhl@...gle.com>, Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Andreas Hindborg <a.hindborg@...sung.com>, Peter Zijlstra <peterz@...radead.org>, Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Arve Hjønnevåg <arve@...roid.com>, Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>, Joel Fernandes <joel@...lfernandes.org>, Carlos Llamas <cmllamas@...gle.com>, Suren Baghdasaryan <surenb@...gle.com>
Cc: Dan Williams <dan.j.williams@...el.com>, Matthew Wilcox <willy@...radead.org>, Thomas Gleixner <tglx@...utronix.de>, Daniel Xu <dxu@...uu.xyz>, Martin Rodriguez Reboredo <yakoyoku@...il.com>, Trevor Gross <tmgross@...ch.edu>, linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, linux-fsdevel@...r.kernel.org, Kees Cook <kees@...nel.org>
Subject: Re: [PATCH v8 3/8] rust: file: add Rust abstraction for `struct file`
On 25.07.24 16:27, Alice Ryhl wrote:
> +/// Wraps the kernel's `struct file`. Not thread safe.
> +///
> +/// This type represents a file that is not known to be safe to transfer across thread boundaries.
> +/// To obtain a thread-safe [`File`], use the [`assume_no_fdget_pos`] conversion.
> +///
> +/// See the documentation for [`File`] for more information.
> +///
> +/// # Invariants
> +///
> +/// * All instances of this type are refcounted using the `f_count` field.
> +/// * If there is an active call to `fdget_pos` that did not take the `f_pos_lock` mutex, then it
> +/// must be on the same thread as this `File`.
Do you mean `LocalFile`?
> +///
> +/// [`assume_no_fdget_pos`]: LocalFile::assume_no_fdget_pos
> +pub struct LocalFile {
> + inner: Opaque<bindings::file>,
> +}
[...]
> + /// Returns the flags associated with the file.
> + ///
> + /// The flags are a combination of the constants in [`flags`].
> + #[inline]
> + pub fn flags(&self) -> u32 {
> + // This `read_volatile` is intended to correspond to a READ_ONCE call.
> + //
> + // SAFETY: The file is valid because the shared reference guarantees a nonzero refcount.
> + //
> + // FIXME(read_once): Replace with `read_once` when available on the Rust side.
Do you know the status of this?
> + unsafe { core::ptr::addr_of!((*self.as_ptr()).f_flags).read_volatile() }
> + }
> +}
> +
> +impl File {
> + /// Creates a reference to a [`File`] from a valid pointer.
> + ///
> + /// # Safety
> + ///
> + /// * The caller must ensure that `ptr` points at a valid file and that the file's refcount is
> + /// positive for the duration of 'a.
> + /// * The caller must ensure that if there are active `fdget_pos` calls on this file, then they
> + /// took the `f_pos_lock` mutex.
> + #[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)`.
> + //
> + // INVARIANT: The caller guarantees that there are no problematic `fdget_pos` calls.
> + unsafe { &*ptr.cast() }
> + }
> +}
> +
> +// Make LocalFile methods available on File.
> +impl core::ops::Deref for File {
> + type Target = LocalFile;
> + #[inline]
> + fn deref(&self) -> &LocalFile {
> + // SAFETY: The caller provides a `&File`, and since it is a reference, it must point at a
> + // valid file for the desired duration.
> + //
> + // By the type invariants, there are no `fdget_pos` calls that did not take the
> + // `f_pos_lock` mutex.
> + unsafe { LocalFile::from_raw_file(self as *const File as *const bindings::file) }
> + }
> +}
> +
> +// SAFETY: The type invariants guarantee that `LocalFile` is always ref-counted. This implementation
> +// makes `ARef<File>` own a normal refcount.
> +unsafe impl AlwaysRefCounted for LocalFile {
> + #[inline]
> + fn inc_ref(&self) {
> + // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> + unsafe { bindings::get_file(self.as_ptr()) };
> + }
> +
> + #[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`.
> + unsafe { bindings::fput(obj.cast().as_ptr()) }
> + }
> +}
Can you move these `AlwaysRefCounted` impls towards the struct
definitions?
With those two comments fixed:
Reviewed-by: Benno Lossin <benno.lossin@...ton.me>
---
Cheers,
Benno
Powered by blists - more mailing lists