linux-kernel - Re: [PATCH v3 03/10] rust: list: add tracking for ListArc

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: <95687c33-4abf-43b7-b835-e18cfae280ef@proton.me>
Date: Wed, 31 Jul 2024 17:17:52 +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 v3 03/10] rust: list: add tracking for ListArc

On 23.07.24 10:22, Alice Ryhl wrote:
> @@ -47,9 +48,30 @@ pub trait ListArcSafe<const ID: u64 = 0> {
>      unsafe fn on_drop_list_arc(&self);
>  }
> 
> +/// Declares that this type is able to safely attempt to create `ListArc`s at any time.
> +///
> +/// # Safety
> +///
> +/// Implementers must ensure that `try_new_list_arc` does not return `true` if a `ListArc` already
> +/// exists.
> +pub unsafe trait TryNewListArc<const ID: u64 = 0>: ListArcSafe<ID> {
> +    /// Attempts to convert an `Arc<Self>` into an `ListArc<Self>`. Returns `true` if the
> +    /// conversion was successful.

I think this could use some more explanation. It has been a couple weeks
since I last reviewed this, so I forgot what exactly this does and at
first I thought that the tracking is not updated. But it actually is.
Additionally, the safety documentation does not ensure that this
function updates the tracking. So maybe use this?:

    Attempts to convert an `Arc<Self>` into a `ListArc<Self>`.

    # Guarantees

    * If the return value is `true`, then there exists no `ListArc<Self, ID>` pointing to this value.
      Additionally, the tracking inside of `self` now thinks there is a `ListArc<Self, ID>`.

And then add in the `Safety` section of the trait that the guarantees of
`try_new_list_arc` need to be ensured.

Just to make sure: this method must handle concurrent access, but that
should be guaranteed if `Self: Sync`, right?

> +    fn try_new_list_arc(&self) -> bool;
> +}
> +
>  /// Declares that this type supports [`ListArc`].
>  ///
> -/// When using this macro, it will only be possible to create a [`ListArc`] from a [`UniqueArc`].
> +/// This macro supports a few different strategies for implementing the tracking inside the type:
> +///
> +/// * The `untracked` strategy does not actually keep track of whether a [`ListArc`] exists. When
> +///   using this strategy, the only way to create a [`ListArc`] is using a [`UniqueArc`].
> +/// * The `tracked_by` strategy defers the tracking to a field of the struct. The user much specify
> +///   which field to defer the tracking to. The field must implement [`ListArcSafe`].
> +///
> +/// The `tracked_by` strategy is usually used by deferring to a field of type
> +/// [`AtomicListArcTracker`]. However, it is also possible to defer the tracking to another struct
> +/// using also using this macro.

Can you add that if the field selected by the `tracked_by` strategy
implements `TryNewListArc`, so will `$t`?

>  #[macro_export]
>  macro_rules! impl_list_arc_safe {
>      (impl$({$($generics:tt)*})? ListArcSafe<$num:tt> for $t:ty { untracked; } $($rest:tt)*) => {
> @@ -60,6 +82,39 @@ unsafe fn on_drop_list_arc(&self) {}
>          $crate::list::impl_list_arc_safe! { $($rest)* }
>      };

[...]

> @@ -346,3 +447,60 @@ impl<T, U, const ID: u64> core::ops::DispatchFromDyn<ListArc<U, ID>> for ListArc
>      U: ListArcSafe<ID> + ?Sized,
>  {
>  }
> +
> +/// A utility for tracking whether a [`ListArc`] exists using an atomic.
> +///
> +/// # Invariant
> +///
> +/// If the boolean is `false`, then there is no [`ListArc`] for this value.
> +#[repr(transparent)]
> +pub struct AtomicListArcTracker<const ID: u64 = 0> {

I am not a fan of this long name, what about `AtomicTracker`? I could
see this type also being used by other things that need tracking.

> +    inner: AtomicBool,
> +    _pin: PhantomPinned,

Would be nice to have a comment explaining why this is needed. IIUC then
it is needed because of the INVARIANT comment in `new()`.

---
Cheers,
Benno

> +}
> +
> +impl<const ID: u64> AtomicListArcTracker<ID> {
> +    /// 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: AtomicBool::new(false),
> +            _pin: PhantomPinned,
> +        }
> +    }