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@google.com>
---
rust/kernel/list.rs | 3 +
rust/kernel/list/impl_list_item_mod.rs | 139 +++++++++++++++++++++++++++++++++
2 files changed, 142 insertions(+)
diff --git a/rust/kernel/list.rs b/rust/kernel/list.rs
index 63e6f6a47964..8fb7d2c13580 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, AtomicListArcTracker, 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..9b1947371c63
--- /dev/null
+++ b/rust/kernel/list/impl_list_item_mod.rs
@@ -0,0 +1,139 @@
+// 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.
+///
+/// # 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.
+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_work!` 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;
+
+/// Implements the [`ListItem`] trait for the given type.
+///
+/// Assumes that the type implements [`HasListLinks`].
+///
+/// [`ListItem`]: crate::list::ListItem
+#[macro_export]
+macro_rules! impl_list_item {
+ (
+ impl$({$($generics:tt)*})? ListItem<$num:tt> for $t:ty {
+ using ListLinks;
+ } $($rest:tt)*
+ ) => {
+ // SAFETY: See GUARANTEES comment on each method.
+ unsafe impl$(<$($generics)*>)? $crate::list::ListItem<$num> for $t {
+ // GUARANTEES:
+ // * This returns the same pointer as `prepare_to_insert` because `prepare_to_insert`
+ // is implemented in terms of `view_links`.
+ // * By the type invariants of `ListLinks`, the `ListLinks` has two null pointers when
+ // this value is not in a list.
+ unsafe fn view_links(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
+ // SAFETY: The caller guarantees that `me` points at a valid value of type `Self`.
+ unsafe {
+ <Self as $crate::list::HasListLinks<$num>>::raw_get_list_links(me.cast_mut())
+ }
+ }
+
+ // GUARANTEES:
+ // * `me` originates from the most recent call to `prepare_to_insert`, which just added
+ // `offset` to the pointer passed to `prepare_to_insert`. This method subtracts
+ // `offset` from `me` so it returns the pointer originally passed to
+ // `prepare_to_insert`.
+ // * The pointer remains valid until the next call to `post_remove` because the caller
+ // of the most recent call to `prepare_to_insert` promised to retain ownership of the
+ // `ListArc` containing `Self` until the next call to `post_remove`. The value cannot
+ // be destroyed while a `ListArc` reference exists.
+ unsafe fn view_value(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
+ let offset = <Self as $crate::list::HasListLinks<$num>>::OFFSET;
+ // SAFETY: `me` originates from the most recent call to `prepare_to_insert`, so it
+ // points at the field at offset `offset` in a value of type `Self`. Thus,
+ // subtracting `offset` from `me` is still in-bounds of the allocation.
+ unsafe { (me as *const u8).sub(offset) as *const Self }
+ }
+
+ // GUARANTEES:
+ // This implementation of `ListItem` will not give out exclusive access to the same
+ // `ListLinks` several times because calls to `prepare_to_insert` and `post_remove`
+ // must alternate and exclusive access is given up when `post_remove` is called.
+ //
+ // Other invocations of `impl_list_item!` also cannot give out exclusive access to the
+ // same `ListLinks` because you can only implement `ListItem` once for each value of
+ // `ID`, and the `ListLinks` fields only work with the specified `ID`.
+ unsafe fn prepare_to_insert(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
+ // SAFETY: The caller promises that `me` points at a valid value.
+ unsafe { <Self as $crate::list::ListItem<$num>>::view_links(me) }
+ }
+
+ // GUARANTEES:
+ // The first guarantee of `view_value` is exactly what `post_remove` guarantees.
+ unsafe fn post_remove(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
+ // SAFETY: This violates the safety requirement that `post_remove` has not been
+ // called since the most recent call to `prepare_to_insert`, but that is okay
+ // because the concrete implementation of `view_value` above does not rely on that
+ // requirement anywhere except for its second guarantee, and we don't need its
+ // second guarantee.
+ unsafe { <Self as $crate::list::ListItem<$num>>::view_value(me) }
+ }
+ }
+ };
+}
+pub use impl_list_item;
--
2.45.2.1089.g2a221341d9-googOn 23.07.24 10:22, Alice Ryhl wrote:
> 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..9b1947371c63
> --- /dev/null
> +++ b/rust/kernel/list/impl_list_item_mod.rs
> @@ -0,0 +1,139 @@
> +// 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.
Can you reference the `impl_has_list_links!` macro here to guide the
user to the safe version?
> +///
> +/// # 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.
> +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_work!` to be correct.
Stale comment (this has nothing to do with `impl_has_work!`).
> + #[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;
> +
> +/// Implements the [`ListItem`] trait for the given type.
> +///
> +/// Assumes that the type implements [`HasListLinks`].
I would write "Requires", since "Assumes" sounds as if it isn't checked.
Can you also reference the `impl_has_list_links!` macro here to guide
the user to the safe version?
> +///
> +/// [`ListItem`]: crate::list::ListItem
> +#[macro_export]
> +macro_rules! impl_list_item {
> + (
> + impl$({$($generics:tt)*})? ListItem<$num:tt> for $t:ty {
> + using ListLinks;
> + } $($rest:tt)*
> + ) => {
> + // SAFETY: See GUARANTEES comment on each method.
> + unsafe impl$(<$($generics)*>)? $crate::list::ListItem<$num> for $t {
> + // GUARANTEES:
> + // * This returns the same pointer as `prepare_to_insert` because `prepare_to_insert`
> + // is implemented in terms of `view_links`.
> + // * By the type invariants of `ListLinks`, the `ListLinks` has two null pointers when
> + // this value is not in a list.
> + unsafe fn view_links(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
> + // SAFETY: The caller guarantees that `me` points at a valid value of type `Self`.
> + unsafe {
> + <Self as $crate::list::HasListLinks<$num>>::raw_get_list_links(me.cast_mut())
> + }
> + }
> +
> + // GUARANTEES:
> + // * `me` originates from the most recent call to `prepare_to_insert`, which just added
> + // `offset` to the pointer passed to `prepare_to_insert`. This method subtracts
> + // `offset` from `me` so it returns the pointer originally passed to
> + // `prepare_to_insert`.
> + // * The pointer remains valid until the next call to `post_remove` because the caller
> + // of the most recent call to `prepare_to_insert` promised to retain ownership of the
> + // `ListArc` containing `Self` until the next call to `post_remove`. The value cannot
> + // be destroyed while a `ListArc` reference exists.
> + unsafe fn view_value(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
> + let offset = <Self as $crate::list::HasListLinks<$num>>::OFFSET;
> + // SAFETY: `me` originates from the most recent call to `prepare_to_insert`, so it
> + // points at the field at offset `offset` in a value of type `Self`. Thus,
> + // subtracting `offset` from `me` is still in-bounds of the allocation.
> + unsafe { (me as *const u8).sub(offset) as *const Self }
> + }
> +
> + // GUARANTEES:
> + // This implementation of `ListItem` will not give out exclusive access to the same
> + // `ListLinks` several times because calls to `prepare_to_insert` and `post_remove`
> + // must alternate and exclusive access is given up when `post_remove` is called.
> + //
> + // Other invocations of `impl_list_item!` also cannot give out exclusive access to the
> + // same `ListLinks` because you can only implement `ListItem` once for each value of
> + // `ID`, and the `ListLinks` fields only work with the specified `ID`.
> + unsafe fn prepare_to_insert(me: *const Self) -> *mut $crate::list::ListLinks<$num> {
> + // SAFETY: The caller promises that `me` points at a valid value.
> + unsafe { <Self as $crate::list::ListItem<$num>>::view_links(me) }
> + }
> +
> + // GUARANTEES:
> + // The first guarantee of `view_value` is exactly what `post_remove` guarantees.
> + unsafe fn post_remove(me: *mut $crate::list::ListLinks<$num>) -> *const Self {
> + // SAFETY: This violates the safety requirement that `post_remove` has not been
> + // called since the most recent call to `prepare_to_insert`, but that is okay
> + // because the concrete implementation of `view_value` above does not rely on that
> + // requirement anywhere except for its second guarantee, and we don't need its
> + // second guarantee.
I don't like the "this isn't correct, but if you look closely at the
implementations, it's fine". Do you think it would be better if you just
copy paste the impl of `view_value`?
---
Cheers,
Benno
> + unsafe { <Self as $crate::list::ListItem<$num>>::view_value(me) }
> + }
> + }
> + };
> +}
> +pub use impl_list_item;
>
> --
> 2.45.2.1089.g2a221341d9-goog
>
On Tue, Jul 23, 2024 at 10:23 AM Alice Ryhl <aliceryhl@google.com> wrote:
> +#[macro_export]
> +macro_rules! impl_list_item {
> + (
> + impl$({$($generics:tt)*})? ListItem<$num:tt> for $t:ty {
> + using ListLinks;
> + } $($rest:tt)*
This uses $($rest:tt)* but does not call itself at the end. This means
that trying to use this macro with several impl blocks results in
additional impl blocks being silently swallowed. The macro should use
repetition properly here.
Alice
© 2016 - 2024 Red Hat, Inc.