Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rust: task: use safe current macro #1053

Open
wants to merge 32 commits into
base: rust-dev
Choose a base branch
from

Conversation

antonio-hickey
Copy link

Fixes: #1047

Refactor the function Task::pid_in_current_ns to use the safe current!() abstraction instead of the bindings::get_current(), and add a function as_raw() on Task for getting access to the raw C pointer (I'm not sure if this part is actually needed, would appreciate if a maintainer could help me out with this part).

fbq and others added 15 commits December 28, 2023 11:15
This is the next upgrade to the Rust toolchain, from 1.74.1 to 1.75.0
(i.e. the latest) [1].

See the upgrade policy [2] and the comments on the first upgrade in
commit 3ed03f4 ("rust: upgrade to Rust 1.68.2").

# Unstable features

The `const_maybe_uninit_zeroed` unstable feature [3] was stabilized in
Rust 1.75.0, which we were using in the PHYLIB abstractions.

The only unstable features allowed to be used outside the `kernel` crate
are still `new_uninit,offset_of`, though other code to be upstreamed
may increase the list.

Please see [4] for details.

# Other improvements

Rust 1.75.0 stabilized `pointer_byte_offsets` [5] which we could
potentially use as an alternative for `ptr_metadata` in the future.

# Required changes

For this upgrade, no changes were required (i.e. on our side).

# `alloc` upgrade and reviewing

The vast majority of changes are due to our `alloc` fork being upgraded
at once.

There are two kinds of changes to be aware of: the ones coming from
upstream, which we should follow as closely as possible, and the updates
needed in our added fallible APIs to keep them matching the newer
infallible APIs coming from upstream.

Instead of taking a look at the diff of this patch, an alternative
approach is reviewing a diff of the changes between upstream `alloc` and
the kernel's. This allows to easily inspect the kernel additions only,
especially to check if the fallible methods we already have still match
the infallible ones in the new version coming from upstream.

Another approach is reviewing the changes introduced in the additions in
the kernel fork between the two versions. This is useful to spot
potentially unintended changes to our additions.

To apply these approaches, one may follow steps similar to the following
to generate a pair of patches that show the differences between upstream
Rust and the kernel (for the subset of `alloc` we use) before and after
applying this patch:

    # Get the difference with respect to the old version.
    git -C rust checkout $(linux/scripts/min-tool-version.sh rustc)
    git -C linux ls-tree -r --name-only HEAD -- rust/alloc |
        cut -d/ -f3- |
        grep -Fv README.md |
        xargs -IPATH cp rust/library/alloc/src/PATH linux/rust/alloc/PATH
    git -C linux diff --patch-with-stat --summary -R > old.patch
    git -C linux restore rust/alloc

    # Apply this patch.
    git -C linux am rust-upgrade.patch

    # Get the difference with respect to the new version.
    git -C rust checkout $(linux/scripts/min-tool-version.sh rustc)
    git -C linux ls-tree -r --name-only HEAD -- rust/alloc |
        cut -d/ -f3- |
        grep -Fv README.md |
        xargs -IPATH cp rust/library/alloc/src/PATH linux/rust/alloc/PATH
    git -C linux diff --patch-with-stat --summary -R > new.patch
    git -C linux restore rust/alloc

Now one may check the `new.patch` to take a look at the additions (first
approach) or at the difference between those two patches (second
approach). For the latter, a side-by-side tool is recommended.

Link: https://github.com/rust-lang/rust/blob/stable/RELEASES.md#version-1750-2023-12-28 [1]
Link: https://rust-for-linux.com/rust-version-policy [2]
Link: rust-lang/rust#91850 [3]
Link: Rust-for-Linux#2 [4]
Link: rust-lang/rust#96283 [5]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Vincenzo Palazzo <vincenzopalazzodev@gmail.com>
Link: https://lore.kernel.org/r/20231224172128.271447-1-ojeda@kernel.org
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 a `from_fd` 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@gmail.com>
Co-developed-by: Daniel Xu <dxu@dxuuu.xyz>
Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/r/20231206-alice-file-v2-1-af617c0d9d94@google.com
[boqun: Resolve conflicts against net-next]
Add a wrapper around `struct cred` called `Credential`, and provide
functionality to get the `Credential` associated with a `File`.

Rust Binder must check the credentials of processes when they attempt to
perform various operations, and these checks usually take a
`&Credential` as parameter. The security_binder_set_context_mgr function
would be one example. This patch is necessary to access these security_*
methods from Rust.

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>
Link: https://lore.kernel.org/r/20231206-alice-file-v2-2-af617c0d9d94@google.com
Adds an abstraction for viewing the string representation of a security
context.

This is needed by Rust Binder because it has feature where a process can
view the string representation of the security context for incoming
transactions. The process can use that to authenticate incoming
transactions, and since the feature is provided by the kernel, the
process can trust that the security context is legitimate.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/r/20231206-alice-file-v2-3-af617c0d9d94@google.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>
Link: https://lore.kernel.org/r/20231206-alice-file-v2-4-af617c0d9d94@google.com
Adds a wrapper around `kuid_t` called `Kuid`. This allows us to define
various operations on kuids such as equality and current_euid. It also
lets us provide conversions from kuid into userspace values.

Rust Binder needs these operations because it needs to compare kuids for
equality, and it needs to tell userspace about the pid and uid of
incoming transactions.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/r/20231206-alice-file-v2-5-af617c0d9d94@google.com
To close an fd from kernel space, we could call `ksys_close`. However,
if we do this to an fd that is held using `fdget`, then we may trigger a
use-after-free. Introduce a helper that can be used to close an fd even
if the fd is currently held with `fdget`. This is done by grabbing an
extra refcount to the file and dropping it in a task work once we return
to userspace.

This is necessary for Rust Binder because otherwise the user might try
to have Binder close its fd for /dev/binder, which would cause problems
as this happens inside an ioctl on /dev/binder, and ioctls hold the fd
using `fdget`.

Additional motivation can be found in commit 80cd795 ("binder: fix
use-after-free due to ksys_close() during fdget()") and in the comments
on `binder_do_fd_close`.

If there is some way to detect whether an fd is currently held with
`fdget`, then this could be optimized to skip the allocation and task
work when this is not the case. Another possible optimization would be
to combine several fds into a single task work, since this is used with
fd arrays that might hold several fds.

That said, it might not be necessary to optimize it, because Rust Binder
has two ways to send fds: BINDER_TYPE_FD and BINDER_TYPE_FDA. With
BINDER_TYPE_FD, it is userspace's responsibility to close the fd, so
this mechanism is used only by BINDER_TYPE_FDA, but fd arrays are used
rarely these days.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/r/20231206-alice-file-v2-6-af617c0d9d94@google.com
[boqun: Resolve conflicts against net-next]
The existing `CondVar` abstraction is a wrapper around `wait_list`, but
it does not support all use-cases of the C `wait_list` type. To be
specific, a `CondVar` cannot be registered with a `struct poll_table`.
This limitation has the advantage that you do not need to call
`synchronize_rcu` when destroying a `CondVar`.

However, we need the ability to register a `poll_table` with a
`wait_list` in Rust Binder. To enable this, introduce a type called
`PollCondVar`, which is like `CondVar` except that you can register a
`poll_table`. We also introduce `PollTable`, which is a safe wrapper
around `poll_table` that is intended to be used with `PollCondVar`.

The destructor of `PollCondVar` unconditionally calls `synchronize_rcu`
to ensure that the removal of epoll waiters has fully completed before
the `wait_list` is destroyed.

That said, `synchronize_rcu` is rather expensive and is not needed in
all cases: If we have never registered a `poll_table` with the
`wait_list`, then we don't need to call `synchronize_rcu`. (And this is
a common case in Binder - not all processes use Binder with epoll.) The
current implementation does not account for this, but if we find that it
is necessary to improve this, a future patch could change store a
boolean next to the `wait_list` to keep track of whether a `poll_table`
has ever been registered.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/r/20231206-alice-file-v2-7-af617c0d9d94@google.com
[ boqun: Removes unused POLLFREE definition ]
The generic parameters on a type definition can specify default values.
Currently `parse_generics()` cannot handle this though. For example when
parsing the following generics:

    <T: Clone, const N: usize = 0>

The `impl_generics` will be set to `T: Clone, const N: usize = 0` and
`ty_generics` will be set to `T, N`. Now using the `impl_generics` on an
impl block:

    impl<$($impl_generics)*> Foo {}

will result in invalid Rust code, because default values are only
available on type definitions.

Therefore add parsing support for generic parameter default values using
a new kind of generics called `decl_generics` and change the old
behavior of `impl_generics` to not contain the generic parameter default
values.

Now `Generics` has three fields:
- `impl_generics`: the generics with bounds
  (e.g. `T: Clone, const N: usize`)
- `decl_generics`: the generics with bounds and default values
  (e.g. `T: Clone, const N: usize = 0`)
- `ty_generics`:  contains the generics without bounds and without
  default values (e.g. `T, N`)

`impl_generics` is designed to be used on `impl<$impl_generics>`,
`decl_generics` for the type definition, so `struct Foo<$decl_generics>`
and `ty_generics` whenever you use the type, so `Foo<$ty_generics>`.

Here is an example that uses all three different types of generics:

    let (Generics { decl_generics, impl_generics, ty_generics }, rest) = parse_generics(input);
    quote! {
        struct Foo<$($decl_generics)*> {
            // ...
        }

        impl<$impl_generics> Foo<$ty_generics> {
            fn foo() {
                // ...
            }
        }
    }

The next commit contains a fix to the `#[pin_data]` macro making it
compatible with generic parameter default values by relying on this new
behavior.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Link: https://lore.kernel.org/r/20231213220447.3613500-1-benno.lossin@proton.me
Previously, the `ForeignOwnable` trait had a method called `borrow_mut`
that was intended to provide mutable access to the inner value. However,
the method accidentally made it possible to change the address of the
object being modified, which usually isn't what we want. (And when we
want that, it can be done by calling `from_foreign` and `into_foreign`,
like how the old `borrow_mut` was implemented.)

In this patch, we introduce an alternate definition of `borrow_mut` that
solves the previous problem. Conceptually, given a pointer type `P` that
implements `ForeignOwnable`, the `borrow_mut` method gives you the same
kind of access as an `&mut P` would, except that it does not let you
change the pointer `P` itself.

This is analogous to how the existing `borrow` method provides the same
kind of access to the inner value as an `&P`.

Note that for types like `Arc`, having an `&mut Arc<T>` only gives you
immutable access to the inner `T`. This is because mutable references
assume exclusive access, but there might be other handles to the same
reference counted value, so the access isn't exclusive. The `Arc` type
implements this by making `borrow_mut` return the same type as `borrow`.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Link: https://lore.kernel.org/r/20230710074642.683831-1-aliceryhl@google.com
Add support for generic parameters defaults in `#[pin_data]` by using
the newly introduced `decl_generics` instead of the `impl_generics`.

Before this would not compile:

    #[pin_data]
    struct Foo<const N: usize = 0> {
        // ...
    }

because it would be expanded to this:

    struct Foo<const N: usize = 0> {
        // ...
    }

    const _: () = {
        struct __ThePinData<const N: usize = 0> {
            __phantom: ::core::marker::PhantomData<fn(Foo<N>) -> Foo<N>>,
        }
        impl<const N: usize = 0> ::core::clone::Clone for __ThePinData<N> {
            fn clone(&self) -> Self {
                *self
            }
        }

        // [...] rest of expansion omitted
    };

The problem is with the `impl<const N: usize = 0>`, since that is
invalid Rust syntax. It should not mention the default value at all,
since default values only make sense on type definitions.

The new `impl_generics` do not contain the default values, thus
generating correct Rust code.

This is used by the next commit that puts `#[pin_data]` on
`kernel::workqueue::Work`.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20231213220447.3613500-2-benno.lossin@proton.me
Eventually we want all architectures to be using the target as defined
by rustc. However currently some architectures can't do that and are
using the target.json specification. This puts in place the foundation
to allow the use of the builtin target definition or a target.json
specification.

Signed-off-by: Jamie Cunliffe <Jamie.Cunliffe@arm.com>
Link: https://lore.kernel.org/r/20231020155056.3495121-2-Jamie.Cunliffe@arm.com
The previous two patches made it possible to add `#[pin_data]` on
structs with default generic parameter values.
This patch makes `Work` use `#[pin_data]` and removes an invocation of
`pin_init_from_closure`. This function is intended as a low level manual
escape hatch, so it is better to rely on the safe `pin_init!` macro.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: https://lore.kernel.org/r/20231213220447.3613500-3-benno.lossin@proton.me
This commit provides the build flags for Rust for AArch64. The core Rust
support already in the kernel does the rest. This enables the PAC ret
and BTI options in the Rust build flags to match the options that are
used when building C.

The Rust samples have been tested with this commit.

Signed-off-by: Jamie Cunliffe <Jamie.Cunliffe@arm.com>
Acked-by: Will Deacon <will@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
Link: https://lore.kernel.org/r/20231020155056.3495121-3-Jamie.Cunliffe@arm.com
@ojeda
Copy link
Member

ojeda commented Jan 3, 2024

Please note this part of the issue text:

This requires submitting a proper patch to the LKML and the Rust for Linux mailing list. Please recall to test your changes, to use a proper title for the commit, to sign your commit under the Developer's Certificate of Origin and so on. Please see https://docs.kernel.org/process/submitting-patches.html and https://rust-for-linux.com/contributing for details.

@antonio-hickey
Copy link
Author

How's it look? Also not sure if I sent the patch to too many people, I used the scripts/get_maintainer.pl for who to send the patch to.

From c1a067128139440ccddc42397974b27e6f7adbc5 Mon Sep 17 00:00:00 2001
From: Antonio Hickey <antoniohickey99@gmail.com>
Date: Tue, 2 Jan 2024 17:14:17 -0500
Subject: [PATCH] rust: task: use safe `current!` macro

Refactor the `Task::pid_in_current_ns()` to use the safe abstraction
`current!()` instead of the unsafe `bindings::get_current()` binding.

Signed-off-by: Antonio Hickey <antoniohickey99@gmail.com>
---
 rust/kernel/task.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
index d2f2615fe4a1..05fc090b1565 100644
--- a/rust/kernel/task.rs
+++ b/rust/kernel/task.rs
@@ -164,7 +164,7 @@ pub fn signal_pending(&self) -> bool {
     /// Returns the given task's pid in the current pid namespace.
     pub fn pid_in_current_ns(&self) -> Pid {
         // SAFETY: Calling `task_active_pid_ns` with the current task is always safe.
-        let namespace = unsafe { bindings::task_active_pid_ns(bindings::get_current()) };
+        let namespace = unsafe { bindings::task_active_pid_ns(current!()) };
         // SAFETY: We know that `self.0.get()` is valid by the type invariant.
         unsafe { bindings::task_tgid_nr_ns(self.0.get(), namespace) }
     }
--
2.34.1

@y86-dev
Copy link
Collaborator

y86-dev commented Jan 3, 2024

Have you tried to compile the kernel with this change? Because this change should not actually work, since task_active_pid_ns requires a *mut task_struct, but current! returns a impl Deref<Target = Task>. So there you would have to call the as_raw function that you had in this PR.

When sending the patch, please also mention that this changes something only in rust-dev, since what you are fixing here is actually not yet upstreamed.

@antonio-hickey
Copy link
Author

antonio-hickey commented Jan 3, 2024

task_active_pid_ns requires a *mut task_struct, but current! returns a impl Deref<Target = Task>. So there you would have to call the as_raw function that you had in this PR.

Ahhh ok this all makes a lot more sense now thanks, and how does updating the patch I sent work?
(is that "amendable" per say, or do I just send a new patch? thanks)

@y86-dev
Copy link
Collaborator

y86-dev commented Jan 4, 2024

Where did you send it to? I do not see it on the mailing list. If you mean on github, then feel free to just amend and force push, since we take the patch via email anyways.

rust/kernel/task.rs Outdated Show resolved Hide resolved
Added new function `Task::as_raw()` which returns the raw pointer
for the underlying task struct. I also refactored `Task` to instead
use the newly created function instead of `self.0.get()` as I feel
like `self.as_raw()` is more intuitive.

Signed-off-by: Antonio Hickey <antoniohickey99@gmail.com>
Refactor the `Task::pid_in_current_ns()` to use the safe abstraction
`current!()` instead of the unsafe `bindings::get_current()` binding.

Signed-off-by: Antonio Hickey <antoniohickey99@gmail.com>
@antonio-hickey
Copy link
Author

When sending the patch, please also mention that this changes something only in rust-dev, since what you are fixing here is actually not yet upstreamed.

I plan on sending the patch this weekend, how should I mention it's for rust-dev like should I update my commit messages or add rust-dev to the patch name? Also since this patch is for code that isn't upstreamed yet would it be better to just sit on this PR until it gets upstreamed to rust-next?

The generic parameters on a type definition can specify default values.
Currently `parse_generics()` cannot handle this though. For example when
parsing the following generics:

    <T: Clone, const N: usize = 0>

The `impl_generics` will be set to `T: Clone, const N: usize = 0` and
`ty_generics` will be set to `T, N`. Now using the `impl_generics` on an
impl block:

    impl<$($impl_generics)*> Foo {}

will result in invalid Rust code, because default values are only
available on type definitions.

Therefore add parsing support for generic parameter default values using
a new kind of generics called `decl_generics` and change the old
behavior of `impl_generics` to not contain the generic parameter default
values.

Now `Generics` has three fields:
- `impl_generics`: the generics with bounds
  (e.g. `T: Clone, const N: usize`)
- `decl_generics`: the generics with bounds and default values
  (e.g. `T: Clone, const N: usize = 0`)
- `ty_generics`:  contains the generics without bounds and without
  default values (e.g. `T, N`)

`impl_generics` is designed to be used on `impl<$impl_generics>`,
`decl_generics` for the type definition, so `struct Foo<$decl_generics>`
and `ty_generics` whenever you use the type, so `Foo<$ty_generics>`.

Here is an example that uses all three different types of generics:

    let (Generics { decl_generics, impl_generics, ty_generics }, rest) = parse_generics(input);
    quote! {
        struct Foo<$($decl_generics)*> {
            // ...
        }

        impl<$impl_generics> Foo<$ty_generics> {
            fn foo() {
                // ...
            }
        }
    }

The next commit contains a fix to the `#[pin_data]` macro making it
compatible with generic parameter default values by relying on this new
behavior.

Signed-off-by: Benno Lossin <benno.lossin@proton.me>
Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>
Link: https://lore.kernel.org/r/20231213220447.3613500-1-benno.lossin@proton.me
Adds a wrapper around `kuid_t` called `Kuid`. This allows us to define
various operations on kuids such as equality and current_euid. It also
lets us provide conversions from kuid into userspace values.

Rust Binder needs these operations because it needs to compare kuids for
equality, and it needs to tell userspace about the pid and uid of
incoming transactions.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/r/20231206-alice-file-v2-5-af617c0d9d94@google.com
[boqun: Resolve conflicts with wait_timeout patchset]
To close an fd from kernel space, we could call `ksys_close`. However,
if we do this to an fd that is held using `fdget`, then we may trigger a
use-after-free. Introduce a helper that can be used to close an fd even
if the fd is currently held with `fdget`. This is done by grabbing an
extra refcount to the file and dropping it in a task work once we return
to userspace.

This is necessary for Rust Binder because otherwise the user might try
to have Binder close its fd for /dev/binder, which would cause problems
as this happens inside an ioctl on /dev/binder, and ioctls hold the fd
using `fdget`.

Additional motivation can be found in commit 80cd795 ("binder: fix
use-after-free due to ksys_close() during fdget()") and in the comments
on `binder_do_fd_close`.

If there is some way to detect whether an fd is currently held with
`fdget`, then this could be optimized to skip the allocation and task
work when this is not the case. Another possible optimization would be
to combine several fds into a single task work, since this is used with
fd arrays that might hold several fds.

That said, it might not be necessary to optimize it, because Rust Binder
has two ways to send fds: BINDER_TYPE_FD and BINDER_TYPE_FDA. With
BINDER_TYPE_FD, it is userspace's responsibility to close the fd, so
this mechanism is used only by BINDER_TYPE_FDA, but fd arrays are used
rarely these days.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/r/20231206-alice-file-v2-6-af617c0d9d94@google.com
[boqun: Resolve conflicts against net-next]
The existing `CondVar` abstraction is a wrapper around `wait_list`, but
it does not support all use-cases of the C `wait_list` type. To be
specific, a `CondVar` cannot be registered with a `struct poll_table`.
This limitation has the advantage that you do not need to call
`synchronize_rcu` when destroying a `CondVar`.

However, we need the ability to register a `poll_table` with a
`wait_list` in Rust Binder. To enable this, introduce a type called
`PollCondVar`, which is like `CondVar` except that you can register a
`poll_table`. We also introduce `PollTable`, which is a safe wrapper
around `poll_table` that is intended to be used with `PollCondVar`.

The destructor of `PollCondVar` unconditionally calls `synchronize_rcu`
to ensure that the removal of epoll waiters has fully completed before
the `wait_list` is destroyed.

That said, `synchronize_rcu` is rather expensive and is not needed in
all cases: If we have never registered a `poll_table` with the
`wait_list`, then we don't need to call `synchronize_rcu`. (And this is
a common case in Binder - not all processes use Binder with epoll.) The
current implementation does not account for this, but if we find that it
is necessary to improve this, a future patch could change store a
boolean next to the `wait_list` to keep track of whether a `poll_table`
has ever been registered.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/r/20231206-alice-file-v2-7-af617c0d9d94@google.com
[ boqun: Removes unused POLLFREE definition ]
@y86-dev
Copy link
Collaborator

y86-dev commented Jan 10, 2024

I would definitely mention it. Whether it is better to wait or not, I am not sure, maybe @ojeda knows something about that?

@ojeda
Copy link
Member

ojeda commented Jan 10, 2024

I would normally wait until the patch can be sent against a Linus' tag or rust-next, but it is not a big deal as long as it is mentioned. :)

@fbq fbq force-pushed the rust-dev branch 2 times, most recently from 9bdbf5d to ddbd3ca Compare January 22, 2024 21:49
@fbq fbq force-pushed the rust-dev branch 3 times, most recently from 7b0b25e to ba2f66a Compare February 5, 2024 18:00
@fbq fbq force-pushed the rust-dev branch 3 times, most recently from 596f12c to 6bdd6e6 Compare February 19, 2024 01:41
@fbq fbq force-pushed the rust-dev branch 3 times, most recently from b27fcc3 to 9a03b3f Compare March 12, 2024 00:07
@fbq fbq force-pushed the rust-dev branch 5 times, most recently from 35767d5 to 6007000 Compare March 29, 2024 23:38
@fbq fbq force-pushed the rust-dev branch 2 times, most recently from 5d0c2b6 to 198cc1a Compare April 10, 2024 17:42
@fbq fbq force-pushed the rust-dev branch 3 times, most recently from 3f27cd3 to 8ccee0c Compare April 18, 2024 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants