[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <33fe126b-7517-4f05-8dc6-911e5f59fa58@proton.me>
Date: Tue, 06 Aug 2024 14:47:16 +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 05/10] rust: list: add macro for implementing ListItem
On 06.08.24 15:58, Alice Ryhl wrote:
> Adds a macro for safely implementing the ListItem trait. As part of the
> implementation of the macro, we also provide a HasListLinks trait
> similar to the workqueue's HasWorkItem trait.
>
> The HasListLinks trait is only necessary if you are implementing
> ListItem using the impl_list_item macro.
>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
> rust/kernel/list.rs | 3 +
> rust/kernel/list/impl_list_item_mod.rs | 143 +++++++++++++++++++++++++++++++++
> 2 files changed, 146 insertions(+)
I found one safety documentation 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 074ae863ff5a..670d53989b8f 100644
> --- a/rust/kernel/list.rs
> +++ b/rust/kernel/list.rs
> @@ -8,6 +8,9 @@
> use crate::types::Opaque;
> use core::ptr;
>
> +mod impl_list_item_mod;
> +pub use self::impl_list_item_mod::{impl_has_list_links, impl_list_item, HasListLinks};
> +
> mod arc;
> pub use self::arc::{impl_list_arc_safe, AtomicTracker, ListArc, ListArcSafe, TryNewListArc};
>
> diff --git a/rust/kernel/list/impl_list_item_mod.rs b/rust/kernel/list/impl_list_item_mod.rs
> new file mode 100644
> index 000000000000..30886e64b8ef
> --- /dev/null
> +++ b/rust/kernel/list/impl_list_item_mod.rs
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Helpers for implementing list traits safely.
> +
> +use crate::list::ListLinks;
> +
> +/// Declares that this type has a `ListLinks<ID>` field at a fixed offset.
> +///
> +/// This trait is only used to help implement `ListItem` safely. If `ListItem` is implemented
> +/// manually, then this trait is not needed. Use the [`impl_has_list_links!`] macro to implement
> +/// this trait.
> +///
> +/// # Safety
> +///
> +/// All values of this type must have a `ListLinks<ID>` field at the given offset.
> +///
> +/// The implementation of `raw_get_list_links` must not be changed.
You require here that the implementation must not be changed, but I
would argue that the implementation is changed in the
`impl_has_list_links!` macro below.
How about 's/implementation/behavior/` in the line above?
---
Cheers,
Benno
> +pub unsafe trait HasListLinks<const ID: u64 = 0> {
> + /// The offset of the `ListLinks` field.
> + const OFFSET: usize;
> +
> + /// Returns a pointer to the [`ListLinks<T, ID>`] field.
> + ///
> + /// # Safety
> + ///
> + /// The provided pointer must point at a valid struct of type `Self`.
> + ///
> + /// [`ListLinks<T, ID>`]: ListLinks
> + // We don't really need this method, but it's necessary for the implementation of
> + // `impl_has_list_links!` to be correct.
> + #[inline]
> + unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut ListLinks<ID> {
> + // SAFETY: The caller promises that the pointer is valid. The implementer promises that the
> + // `OFFSET` constant is correct.
> + unsafe { (ptr as *mut u8).add(Self::OFFSET) as *mut ListLinks<ID> }
> + }
> +}
> +
> +/// Implements the [`HasListLinks`] trait for the given type.
> +#[macro_export]
> +macro_rules! impl_has_list_links {
> + ($(impl$(<$($implarg:ident),*>)?
> + HasListLinks$(<$id:tt>)?
> + for $self:ident $(<$($selfarg:ty),*>)?
> + { self$(.$field:ident)* }
> + )*) => {$(
> + // SAFETY: The implementation of `raw_get_list_links` only compiles if the field has the
> + // right type.
> + //
> + // The implementation of `raw_get_list_links` is not changed since the `addr_of_mut!` macro
> + // is equivalent to the pointer offset operation in the trait definition.
> + unsafe impl$(<$($implarg),*>)? $crate::list::HasListLinks$(<$id>)? for
> + $self $(<$($selfarg),*>)?
> + {
> + const OFFSET: usize = ::core::mem::offset_of!(Self, $($field).*) as usize;
> +
> + #[inline]
> + unsafe fn raw_get_list_links(ptr: *mut Self) -> *mut $crate::list::ListLinks$(<$id>)? {
> + // SAFETY: The caller promises that the pointer is not dangling. We know that this
> + // expression doesn't follow any pointers, as the `offset_of!` invocation above
> + // would otherwise not compile.
> + unsafe { ::core::ptr::addr_of_mut!((*ptr)$(.$field)*) }
> + }
> + }
> + )*};
> +}
> +pub use impl_has_list_links;
Powered by blists - more mailing lists