Message-ID: <CAH5fLgjsoQP_7=fxHY13Oz+o3zyr7_iFwF_h9PSeUs5Kr5cn6A@mail.gmail.com>
Date: Thu, 8 Feb 2024 16:05:30 +0100
From: Alice Ryhl <aliceryhl@...gle.com>
To: Trevor Gross <tmgross@...ch.edu>
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>, 
	Benno Lossin <benno.lossin@...ton.me>, 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>, 
	Kees Cook <keescook@...omium.org>, Matthew Wilcox <willy@...radead.org>, 
	Thomas Gleixner <tglx@...utronix.de>, Daniel Xu <dxu@...uu.xyz>, linux-kernel@...r.kernel.org, 
	rust-for-linux@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v4 3/9] rust: file: add Rust abstraction for `struct file`

On Tue, Feb 6, 2024 at 3:48 AM Trevor Gross <tmgross@...ch.edu> wrote:
>
> On Fri, Feb 2, 2024 at 5:56 AM Alice Ryhl <aliceryhl@...gle.com> wrote:
> >
> > From: Wedson Almeida Filho <wedsonaf@...il.com>
> >
> > This abstraction makes it possible to manipulate the open files for a
> > process. The new `File` struct wraps the C `struct file`. When accessing
> > it using the smart pointer `ARef<File>`, the pointer will own a
> > reference count to the file. When accessing it as `&File`, then the
> > reference does not own a refcount, but the borrow checker will ensure
> > that the reference count does not hit zero while the `&File` is live.
> >
> > Since this is intended to manipulate the open files of a process, we
> > introduce an `fget` constructor that corresponds to the C `fget`
> > method. In future patches, it will become possible to create a new fd in
> > a process and bind it to a `File`. Rust Binder will use these to send
> > fds from one process to another.
> >
> > We also provide a method for accessing the file's flags. Rust Binder
> > will use this to access the flags of the Binder fd to check whether the
> > non-blocking flag is set, which affects what the Binder ioctl does.
> >
> > This introduces a struct for the EBADF error type, rather than just
> > using the Error type directly. This has two advantages:
> > * `File::from_fd` returns a `Result<ARef<File>, BadFdError>`, which the
> >   compiler will represent as a single pointer, with null being an error.
> >   This is possible because the compiler understands that `BadFdError`
> >   has only one possible value, and it also understands that the
> >   `ARef<File>` smart pointer is guaranteed non-null.
> > * Additionally, we promise to users of the method that the method can
> >   only fail with EBADF, which means that they can rely on this promise
> >   without having to inspect its implementation.
> > That said, there are also two disadvantages:
> > * Defining additional error types involves boilerplate.
> > * The question mark operator will only utilize the `From` trait once,
> >   which prevents you from using the question mark operator on
> >   `BadFdError` in methods that return some third error type that the
> >   kernel `Error` is convertible into. (However, it works fine in methods
> >   that return `Error`.)
> >
> > Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com>
> > Co-developed-by: Daniel Xu <dxu@...uu.xyz>
> > Signed-off-by: Daniel Xu <dxu@...uu.xyz>
> > Co-developed-by: Alice Ryhl <aliceryhl@...gle.com>
> > Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> > ---
> >  fs/file.c                       |   7 +
> >  rust/bindings/bindings_helper.h |   2 +
> >  rust/helpers.c                  |   7 +
> >  rust/kernel/file.rs             | 249 ++++++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs              |   1 +
> >  5 files changed, 266 insertions(+)
> >  create mode 100644 rust/kernel/file.rs
> >
> > diff --git a/fs/file.c b/fs/file.c
> > index 3b683b9101d8..f2eab5fcb87f 100644
> > --- a/fs/file.c
> > +++ b/fs/file.c
> > @@ -1115,18 +1115,25 @@ EXPORT_SYMBOL(task_lookup_next_fdget_rcu);
> >  /*
> >   * Lightweight file lookup - no refcnt increment if fd table isn't shared.
> >   *
> >   * You can use this instead of fget if you satisfy all of the following
> >   * conditions:
> >   * 1) You must call fput_light before exiting the syscall and returning control
> >   *    to userspace (i.e. you cannot remember the returned struct file * after
> >   *    returning to userspace).
> >   * 2) You must not call filp_close on the returned struct file * in between
> >   *    calls to fget_light and fput_light.
> >   * 3) You must not clone the current task in between the calls to fget_light
> >   *    and fput_light.
> >   *
> >   * The fput_needed flag returned by fget_light should be passed to the
> >   * corresponding fput_light.
> > + *
> > + * (As an exception to rule 2, you can call filp_close between fget_light and
> > + * fput_light provided that you capture a real refcount with get_file before
> > + * the call to filp_close, and ensure that this real refcount is fput *after*
> > + * the fput_light call.)
> > + *
> > + * See also the documentation in rust/kernel/file.rs.
> >   */
> >  static unsigned long __fget_light(unsigned int fd, fmode_t mask)
> >  {
>
> Should this be split to its own patch so it can be applied separately if needed?

I don't think that's necessary.

> > [...]
> > +    /// Also known as `O_NDELAY`.
> > +    ///
> > +    /// This is effectively the same flag as [`O_NONBLOCK`] on all architectures
> > +    /// except SPARC64.
> > +    pub const O_NDELAY: u32 = bindings::O_NDELAY;
>
> This is O_NDELAY, should the AKA say O_NONBLOCK?
> > [...]
> > +/// Wraps the kernel's `struct file`.
>
> It is probably better to say what it does for the summary, and mention
> what it wraps later.

I added a bit more info.

> > +/// # Refcounting
> > +///
> > +/// Instances of this type are reference-counted. The reference count is incremented by the
> > +/// `fget`/`get_file` functions and decremented by `fput`. The Rust type `ARef<File>` represents a
> > +/// pointer that owns a reference count on the file.
> > +///
> > +/// Whenever a process opens a file descriptor (fd), it stores a pointer to the file in its `struct
> > +/// files_struct`. This pointer owns a reference count to the file, ensuring the file isn't
> > +/// prematurely deleted while the file descriptor is open. In Rust terminology, the pointers in
> > +/// `struct files_struct` are `ARef<File>` pointers.
> > +///
> > +/// ## Light refcounts
> > +///
> > +/// Whenever a process has an fd to a file, it may use something called a "light refcount" as a
> > +/// performance optimization. Light refcounts are acquired by calling `fdget` and released with
> > +/// `fdput`. The idea behind light refcounts is that if the fd is not closed between the calls to
> > +/// `fdget` and `fdput`, then the refcount cannot hit zero during that time, as the `struct
> > +/// files_struct` holds a reference until the fd is closed. This means that it's safe to access the
> > +/// file even if `fdget` does not increment the refcount.
> > +///
> > +/// The requirement that the fd is not closed during a light refcount applies globally across all
> > +/// threads - not just on the thread using the light refcount. For this reason, light refcounts are
> > +/// only used when the `struct files_struct` is not shared with other threads, since this ensures
> > +/// that other unrelated threads cannot suddenly start using the fd and close it. Therefore,
> > +/// calling `fdget` on a shared `struct files_struct` creates a normal refcount instead of a light
> > +/// refcount.
> > +///
> > +/// Light reference counts must be released with `fdput` before the system call returns to
> > +/// userspace. This means that if you wait until the current system call returns to userspace, then
> > +/// all light refcounts that existed at the time have gone away.
> > +///
> > +/// ## Rust references
> > +///
> > +/// The reference type `&File` is similar to light refcounts:
> > +///
> > +/// * `&File` references don't own a reference count. They can only exist as long as the reference
> > +///   count stays positive, and can only be created when there is some mechanism in place to ensure
> > +///   this.
> > +///
> > +/// * The Rust borrow-checker normally ensures this by enforcing that the `ARef<File>` from which
> > +///   a `&File` is created outlives the `&File`.
> > +///
> > +/// * Using the unsafe [`File::from_ptr`] means that it is up to the caller to ensure that the
> > +///   `&File` only exists while the reference count is positive.
> > +///
> > +/// * You can think of `fdget` as using an fd to look up an `ARef<File>` in the `struct
> > +///   files_struct` and create an `&File` from it. The "fd cannot be closed" rule is like the Rust
> > +///   rule "the `ARef<File>` must outlive the `&File`".
> > +///
> > +/// # Invariants
> > +///
> > +/// * Instances of this type are refcounted using the `f_count` field.
> > +/// * If an fd with active light refcounts is closed, then it must be the case that the file
> > +///   refcount is positive until all light refcounts of the fd have been dropped.
> > +/// * A light refcount must be dropped before returning to userspace.
> > +#[repr(transparent)]
> > +pub struct File(Opaque<bindings::file>);
> > +
> > +// SAFETY:
> > +// - `File::dec_ref` can be called from any thread.
> > +// - It is okay to send ownership of `File` across thread boundaries.
>
> Shouldn't this be lowecase `file` because it is referring to the
> underlying C object?

Done.

> > +unsafe impl Send for File {}
> > [...]
> > +    /// Returns the flags associated with the file.
> > +    ///
> > +    /// The flags are a combination of the constants in [`flags`].
> > +    pub fn flags(&self) -> u32 {
>
> A typedef used here and in the constants module could be useful
>
>     type FileFlags = u32;

Maybe, but it doesn't actually do anything as far as the type checker
is concerned, so meh.

> > +        // 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.
> > +        //
> > +        // TODO: Replace with `read_once` when available on the Rust side.
>
> Shouldn't the TODO become a `FIXME(read_once): ...` since it is going
> into the codebase?

I'm not sure we necessarily have a convention for that? But I'll change it.

>
> Some suggestions but nothing blocking
>
> Reviewed-by: Trevor Gross <tmgross@...ch.edu>

Powered by blists - more mailing lists