Re: [PATCH v3 05/10] rust: list: add macro for implementing ListItem

Benno Lossin
2024-07-31T20:17:17Z

On 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
>