On Wed, Nov 29, 2023 at 01:11:56PM +0000, Alice Ryhl wrote:
> From: Wedson Almeida Filho <wedsonaf@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> Co-developed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
> ---
> 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
>
