From: Christian Brauner <brauner@kernel.org>
To: Alice Ryhl <aliceryhl@google.com>
Cc: "Miguel Ojeda" <ojeda@kernel.org>,
"Alex Gaynor" <alex.gaynor@gmail.com>,
"Wedson Almeida Filho" <wedsonaf@gmail.com>,
"Boqun Feng" <boqun.feng@gmail.com>,
"Gary Guo" <gary@garyguo.net>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Benno Lossin" <benno.lossin@proton.me>,
"Andreas Hindborg" <a.hindborg@samsung.com>,
"Peter Zijlstra" <peterz@infradead.org>,
"Alexander Viro" <viro@zeniv.linux.org.uk>,
"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
"Arve Hjønnevåg" <arve@android.com>,
"Todd Kjos" <tkjos@android.com>,
"Martijn Coenen" <maco@android.com>,
"Joel Fernandes" <joel@joelfernandes.org>,
"Carlos Llamas" <cmllamas@google.com>,
"Suren Baghdasaryan" <surenb@google.com>,
"Dan Williams" <dan.j.williams@intel.com>,
"Kees Cook" <keescook@chromium.org>,
"Matthew Wilcox" <willy@infradead.org>,
"Thomas Gleixner" <tglx@linutronix.de>,
"Daniel Xu" <dxu@dxuuu.xyz>,
linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org,
linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 4/7] rust: file: add `FileDescriptorReservation`
Date: Wed, 29 Nov 2023 17:14:14 +0100 [thread overview]
Message-ID: <20231129-zwiespalt-exakt-f1446d88a62a@brauner> (raw)
In-Reply-To: <20231129-alice-file-v1-4-f81afe8c7261@google.com>
On Wed, Nov 29, 2023 at 01:11:56PM +0000, Alice Ryhl wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
>
> Allow for the creation of a file descriptor in two steps: first, we
> reserve a slot for it, then we commit or drop the reservation. The first
> step may fail (e.g., the current process ran out of available slots),
> but commit and drop never fail (and are mutually exclusive).
>
> This is needed by Rust Binder when fds are sent from one process to
> another. It has to be a two-step process to properly handle the case
> where multiple fds are sent: The operation must fail or succeed
> atomically, which we achieve by first reserving the fds we need, and
> only installing the files once we have reserved enough fds to send the
> files.
>
> Fd reservations assume that the value of `current` does not change
> between the call to get_unused_fd_flags and the call to fd_install (or
> put_unused_fd). By not implementing the Send trait, this abstraction
> ensures that the `FileDescriptorReservation` cannot be moved into a
> different process.
>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
> rust/kernel/file.rs | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs
> index f1f71c3d97e2..2186a6ea3f2f 100644
> --- a/rust/kernel/file.rs
> +++ b/rust/kernel/file.rs
> @@ -11,7 +11,7 @@
> error::{code::*, Error, Result},
> types::{ARef, AlwaysRefCounted, Opaque},
> };
> -use core::ptr;
> +use core::{marker::PhantomData, ptr};
>
> /// Flags associated with a [`File`].
> pub mod flags {
> @@ -180,6 +180,68 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> }
> }
>
> +/// A file descriptor reservation.
> +///
> +/// This allows the creation of a file descriptor in two steps: first, we reserve a slot for it,
> +/// then we commit or drop the reservation. The first step may fail (e.g., the current process ran
> +/// out of available slots), but commit and drop never fail (and are mutually exclusive).
> +///
> +/// Dropping the reservation happens in the destructor of this type.
> +///
> +/// # Invariants
> +///
> +/// The fd stored in this struct must correspond to a reserved file descriptor of the current task.
> +pub struct FileDescriptorReservation {
Can we follow the traditional file terminology, i.e.,
get_unused_fd_flags() and fd_install()? At least at the beginning this
might be quite helpful instead of having to mentally map new() and
commit() onto the C functions.
> + fd: u32,
> + /// Prevent values of this type from being moved to a different task.
> + ///
> + /// This is necessary because the C FFI calls assume that `current` is set to the task that
> + /// owns the fd in question.
> + _not_send_sync: PhantomData<*mut ()>,
I don't fully understand this. Can you explain in a little more detail
what you mean by this and how this works?
> +}
> +
> +impl FileDescriptorReservation {
> + /// Creates a new file descriptor reservation.
> + pub fn new(flags: u32) -> Result<Self> {
> + // SAFETY: FFI call, there are no safety requirements on `flags`.
> + let fd: i32 = unsafe { bindings::get_unused_fd_flags(flags) };
> + if fd < 0 {
> + return Err(Error::from_errno(fd));
> + }
> + Ok(Self {
> + fd: fd as _,
This is a cast to a u32?
> + _not_send_sync: PhantomData,
Can you please draft a quick example how that return value would be
expected to be used by a caller? It's really not clear
> + })
> + }
> +
> + /// Returns the file descriptor number that was reserved.
> + pub fn reserved_fd(&self) -> u32 {
> + self.fd
> + }
> +
> + /// Commits the reservation.
> + ///
> + /// The previously reserved file descriptor is bound to `file`. This method consumes the
> + /// [`FileDescriptorReservation`], so it will not be usable after this call.
> + pub fn commit(self, file: ARef<File>) {
> + // SAFETY: `self.fd` was previously returned by `get_unused_fd_flags`, and `file.ptr` is
> + // guaranteed to have an owned ref count by its type invariants.
> + unsafe { bindings::fd_install(self.fd, file.0.get()) };
Why file.0.get()? Where did that come from?
> +
> + // `fd_install` consumes both the file descriptor and the file reference, so we cannot run
> + // the destructors.
> + core::mem::forget(self);
> + core::mem::forget(file);
> + }
> +}
> +
> +impl Drop for FileDescriptorReservation {
> + fn drop(&mut self) {
> + // SAFETY: `self.fd` was returned by a previous call to `get_unused_fd_flags`.
> + unsafe { bindings::put_unused_fd(self.fd) };
> + }
> +}
> +
> /// Represents the `EBADF` error code.
> ///
> /// Used for methods that can only fail with `EBADF`.
>
> --
> 2.43.0.rc1.413.gea7ed67945-goog
>
next prev parent reply other threads:[~2023-11-29 16:14 UTC|newest]
Thread overview: 96+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-29 12:51 [PATCH 0/7] File abstractions needed by Rust Binder Alice Ryhl
2023-11-29 12:51 ` [PATCH 1/7] rust: file: add Rust abstraction for `struct file` Alice Ryhl
2023-11-29 15:13 ` Matthew Wilcox
2023-11-29 15:23 ` Peter Zijlstra
2023-11-29 17:08 ` Boqun Feng
2023-11-30 10:42 ` Peter Zijlstra
2023-11-30 15:25 ` Boqun Feng
2023-12-01 8:53 ` Peter Zijlstra
2023-12-01 9:19 ` Boqun Feng
2023-12-01 9:40 ` Peter Zijlstra
2023-12-01 10:36 ` Boqun Feng
2023-12-01 11:05 ` Peter Zijlstra
2023-12-01 9:00 ` Peter Zijlstra
2023-12-01 9:52 ` Boqun Feng
2023-11-29 16:42 ` Alice Ryhl
2023-11-29 16:45 ` Peter Zijlstra
2023-11-30 15:02 ` Benno Lossin
2023-11-29 17:06 ` Christian Brauner
2023-11-29 21:27 ` Alice Ryhl
2023-11-29 23:17 ` Benno Lossin
2023-11-30 10:48 ` Christian Brauner
2023-11-30 12:10 ` Alice Ryhl
2023-11-30 12:36 ` Christian Brauner
2023-11-30 14:53 ` Benno Lossin
2023-11-30 14:59 ` Greg Kroah-Hartman
2023-11-30 15:46 ` Benno Lossin
2023-11-30 15:56 ` Greg Kroah-Hartman
2023-11-30 15:58 ` Theodore Ts'o
2023-11-30 16:12 ` Benno Lossin
2023-12-01 1:16 ` Theodore Ts'o
2023-12-01 12:11 ` David Laight
2023-12-01 12:27 ` Alice Ryhl
2023-12-01 15:04 ` Theodore Ts'o
2023-12-01 15:14 ` Benno Lossin
2023-12-01 17:25 ` David Laight
2023-12-01 17:37 ` Benno Lossin
2023-11-29 12:51 ` [PATCH 2/7] rust: cred: add Rust abstraction for `struct cred` Alice Ryhl
2023-11-30 16:17 ` Benno Lossin
2023-12-01 9:06 ` Alice Ryhl
2023-12-01 10:27 ` Christian Brauner
2023-12-04 15:42 ` Alice Ryhl
2023-11-29 13:11 ` [PATCH 3/7] rust: security: add abstraction for secctx Alice Ryhl
2023-11-30 16:26 ` Benno Lossin
2023-12-01 10:48 ` Alice Ryhl
2023-12-02 10:03 ` Benno Lossin
2023-11-29 13:11 ` [PATCH 4/7] rust: file: add `FileDescriptorReservation` Alice Ryhl
2023-11-29 16:14 ` Christian Brauner [this message]
2023-11-29 16:55 ` Alice Ryhl
2023-11-29 17:14 ` Alice Ryhl
2023-11-30 9:12 ` Christian Brauner
2023-11-30 9:23 ` Alice Ryhl
2023-11-30 9:09 ` Christian Brauner
2023-11-30 9:17 ` Alice Ryhl
2023-11-30 10:51 ` Christian Brauner
2023-11-30 11:54 ` Alice Ryhl
2023-11-30 12:17 ` Benno Lossin
2023-11-30 12:33 ` Christian Brauner
2023-11-30 16:40 ` Benno Lossin
2023-12-01 11:32 ` Alice Ryhl
2023-11-29 13:12 ` [PATCH 5/7] rust: file: add `Kuid` wrapper Alice Ryhl
2023-11-29 16:28 ` Christian Brauner
2023-11-29 16:48 ` Peter Zijlstra
2023-11-30 12:46 ` Christian Brauner
2023-12-06 19:59 ` Kent Overstreet
2023-12-08 16:26 ` Peter Zijlstra
2023-12-08 19:58 ` Kent Overstreet
2023-12-08 5:28 ` comex
2023-12-08 16:19 ` Miguel Ojeda
2023-12-08 17:08 ` Nick Desaulniers
2023-12-08 17:37 ` Miguel Ojeda
2023-12-08 17:43 ` Boqun Feng
2023-12-08 20:43 ` Matthew Wilcox
2023-12-09 7:24 ` comex
2023-11-30 9:36 ` Alice Ryhl
2023-11-30 10:52 ` Christian Brauner
2023-11-30 10:36 ` Peter Zijlstra
2023-12-06 20:02 ` Kent Overstreet
2023-12-07 7:18 ` Greg Kroah-Hartman
2023-12-07 7:46 ` Kent Overstreet
2023-11-30 16:48 ` Benno Lossin
2023-11-29 13:12 ` [PATCH 6/7] rust: file: add `DeferredFdCloser` Alice Ryhl
2023-11-30 17:12 ` Benno Lossin
2023-12-01 11:35 ` Alice Ryhl
2023-12-02 10:16 ` Benno Lossin
2023-12-05 14:43 ` Alice Ryhl
2023-12-05 18:16 ` Alice Ryhl
2023-11-29 13:12 ` [PATCH 7/7] rust: file: add abstraction for `poll_table` Alice Ryhl
2023-11-30 17:42 ` Benno Lossin
2023-12-01 11:47 ` Alice Ryhl
2023-11-30 22:39 ` Boqun Feng
2023-12-01 11:50 ` Alice Ryhl
2023-11-30 22:50 ` Boqun Feng
2023-11-29 16:31 ` [PATCH 0/7] File abstractions needed by Rust Binder Christian Brauner
2023-11-29 16:48 ` Miguel Ojeda
2023-12-06 20:05 ` Kent Overstreet
2023-12-08 16:59 ` Miguel Ojeda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231129-zwiespalt-exakt-f1446d88a62a@brauner \
--to=brauner@kernel.org \
--cc=a.hindborg@samsung.com \
--cc=alex.gaynor@gmail.com \
--cc=aliceryhl@google.com \
--cc=arve@android.com \
--cc=benno.lossin@proton.me \
--cc=bjorn3_gh@protonmail.com \
--cc=boqun.feng@gmail.com \
--cc=cmllamas@google.com \
--cc=dan.j.williams@intel.com \
--cc=dxu@dxuuu.xyz \
--cc=gary@garyguo.net \
--cc=gregkh@linuxfoundation.org \
--cc=joel@joelfernandes.org \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maco@android.com \
--cc=ojeda@kernel.org \
--cc=peterz@infradead.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=surenb@google.com \
--cc=tglx@linutronix.de \
--cc=tkjos@android.com \
--cc=viro@zeniv.linux.org.uk \
--cc=wedsonaf@gmail.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).