linux-kernel - Re: [PATCH v8 3/8] rust: file: add Rust abstraction for `struct file`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAH5fLgi0MGUhbD0WV99NtU+08HCJG+LYMtx+Ca4gwfo9FR+hTw@mail.gmail.com>
Date: Tue, 6 Aug 2024 10:48:11 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: 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>, 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 Tue, Aug 6, 2024 at 10:44 AM Benno Lossin <benno.lossin@...ton.me> wrote:
>
> 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`?

I guess. Perhaps I should just say "file" as a general concept?

> > +///
> > +/// [`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?

It's still unavailable.

> > +        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>

Thanks!

Alice