linux-kernel - Re: [PATCH v4 09/10] rust: list: support heterogeneous lists

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: <4858fbdc-f1d4-48bf-b884-4db1b1ffbf1b@proton.me>
Date: Tue, 06 Aug 2024 14:47:21 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Alice Ryhl <aliceryhl@...gle.com>, Miguel Ojeda <ojeda@...nel.org>, Andrew Morton <akpm@...ux-foundation.org>
Cc: 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>, Marco Elver <elver@...gle.com>, Coly Li <colyli@...e.de>, Paolo Abeni <pabeni@...hat.com>, Pierre Gondois <pierre.gondois@....com>, Ingo Molnar <mingo@...nel.org>, Jakub Kicinski <kuba@...nel.org>, Wei Yang <richard.weiyang@...il.com>, Matthew Wilcox <willy@...radead.org>, linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, Kees Cook <kees@...nel.org>
Subject: Re: [PATCH v4 09/10] rust: list: support heterogeneous lists

On 06.08.24 15:59, Alice Ryhl wrote:
> Support linked lists that can hold many different structs at once. This
> is generally done using trait objects. The main challenge is figuring
> what the struct is given only a pointer to the ListLinks.
> 
> We do this by storing a pointer to the struct next to the ListLinks
> field. The container_of operation will then just read that pointer. When
> the type is a trait object, that pointer will be a fat pointer whose
> metadata is a vtable that tells you what kind of struct it is.
> 
> Heterogeneous lists are heavily used by Rust Binder. There are a lot of
> so-called todo lists containing various events that need to be delivered
> to userspace next time userspace calls into the driver. And there are
> quite a few different todo item types: incoming transaction, changes to
> refcounts, death notifications, and more.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
>  rust/kernel/list.rs                    |  47 +++++++++++-
>  rust/kernel/list/impl_list_item_mod.rs | 131 +++++++++++++++++++++++++++++++++
>  2 files changed, 177 insertions(+), 1 deletion(-)

One nit below, with that fixed,

Reviewed-by: Benno Lossin <benno.lossin@...ton.me>

> diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
> index 904cfa454dff..cf26fec37c1e 100644
> --- a/rust/kernel/list.rs
> +++ b/rust/kernel/list.rs
> @@ -12,7 +12,9 @@
>  use core::ptr;
> 
>  mod impl_list_item_mod;
> -pub use self::impl_list_item_mod::{impl_has_list_links, impl_list_item, HasListLinks};
> +pub use self::impl_list_item_mod::{
> +    impl_has_list_links, impl_has_list_links_self_ptr, impl_list_item, HasListLinks, HasSelfPtr,
> +};
> 
>  mod arc;
>  pub use self::arc::{impl_list_arc_safe, AtomicTracker, ListArc, ListArcSafe, TryNewListArc};
> @@ -183,6 +185,49 @@ unsafe fn from_fields(me: *mut ListLinksFields) -> *mut Self {
>      }
>  }
> 
> +/// Similar to [`ListLinks`], but also contains a pointer to the full value.
> +///
> +/// This type can be used instead of [`ListLinks`] to support lists with trait objects.
> +#[repr(C)]
> +pub struct ListLinksSelfPtr<T: ?Sized, const ID: u64 = 0> {
> +    /// The `ListLinks` field inside this value.
> +    ///
> +    /// This is public so that it can be used with `impl_has_list_links!`.
> +    pub inner: ListLinks<ID>,
> +    // UnsafeCell is not enough here because we need `Opaque::zeroed` as a dummy value, and

You say that you need `Opaque::zeroed`, but below you use `uninit`, so
just 's/zeroed/uninit/' in the above line should fix it.

---
Cheers,
Benno

> +    // `ptr::null()` doesn't work for `T: ?Sized`.
> +    self_ptr: Opaque<*const T>,
> +}
> +
> +// SAFETY: The fields of a ListLinksSelfPtr can be moved across thread boundaries.
> +unsafe impl<T: ?Sized + Send, const ID: u64> Send for ListLinksSelfPtr<T, ID> {}
> +// SAFETY: The type is opaque so immutable references to a ListLinksSelfPtr are useless. Therefore,
> +// it's okay to have immutable access to a ListLinks from several threads at once.
> +//
> +// Note that `inner` being a public field does not prevent this type from being opaque, since
> +// `inner` is a opaque type.
> +unsafe impl<T: ?Sized + Sync, const ID: u64> Sync for ListLinksSelfPtr<T, ID> {}
> +
> +impl<T: ?Sized, const ID: u64> ListLinksSelfPtr<T, ID> {
> +    /// The offset from the [`ListLinks`] to the self pointer field.
> +    pub const LIST_LINKS_SELF_PTR_OFFSET: usize = core::mem::offset_of!(Self, self_ptr);
> +
> +    /// Creates a new initializer for this type.
> +    pub fn new() -> impl PinInit<Self> {
> +        // INVARIANT: Pin-init initializers can't be used on an existing `Arc`, so this value will
> +        // not be constructed in an `Arc` that already has a `ListArc`.
> +        Self {
> +            inner: ListLinks {
> +                inner: Opaque::new(ListLinksFields {
> +                    prev: ptr::null_mut(),
> +                    next: ptr::null_mut(),
> +                }),
> +            },
> +            self_ptr: Opaque::uninit(),
> +        }
> +    }
> +}
> +
>  impl<T: ?Sized + ListItem<ID>, const ID: u64> List<T, ID> {
>      /// Creates a new empty list.
>      pub const fn new() -> Self {