linux-kernel - Re: [PATCH 2/9] 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]

Date: Wed, 03 Apr 2024 15:52:45 +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>, Kees Cook <keescook@...omium.org>, 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
Subject: Re: [PATCH 2/9] rust: list: add tracking for ListArc

I think the commit one-line description sounds a bit strange, how about
"rust: list: add ListArc tracking strategies"?

On 02.04.24 14:16, Alice Ryhl wrote:
> @@ -33,19 +34,64 @@ 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.
> +    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`].
> +/// When using this macro, you may choose between the `untracked` strategy where it is not tracked
> +/// whether a [`ListArc`] exists, and the `tracked_by` strategy where the tracking is deferred to a
> +/// field of the struct. The `tracked_by` strategy can be combined with a field of type
> +/// [`AtomicListArcTracker`] to track whether a [`ListArc`] exists.
>  #[macro_export]
>  macro_rules! impl_list_arc_safe {
>      (impl$({$($generics:tt)*})? ListArcSafe<$num:tt> for $t:ty { untracked; } $($rest:tt)*) => {
> -        impl$(<$($generics)*>)? $crate::list::ListArcSafe<$num> for $t {
> +        impl$(<$($generics)*>)? ListArcSafe<$num> for $t {

This change seems unintentional.

>              unsafe fn on_create_list_arc_from_unique(&mut self) {}
>              unsafe fn on_drop_list_arc(&self) {}
>          }
>          $crate::list::impl_list_arc_safe! { $($rest)* }
>      };
> 
> +    (impl$({$($generics:tt)*})? ListArcSafe<$num:tt> for $t:ty {
> +        tracked_by $field:ident : $fty:ty;
> +    } $($rest:tt)*) => {
> +        impl$(<$($generics)*>)? ListArcSafe<$num> for $t {

Here you also want to access `ListArcSafe` via
`$crate::list::ListArcSafe`.

> +            unsafe fn on_create_list_arc_from_unique(&mut self) {
> +                let me = self as *mut Self;
> +                let field: *mut $fty = unsafe { ::core::ptr::addr_of_mut!((*me).$field) };

I think we should also have `SAFETY` comments in macros.

Also why can't this be done using safe code?:

     let field: &mut $fty = &mut self.$field;

> +                unsafe { <$fty as $crate::list::ListArcSafe<$num>>::on_create_list_arc_from_unique(
> +                        &mut *field
> +                ) };

Formatting? rustfmt gives me this:

                 unsafe {
                     <$fty as $crate::list::ListArcSafe<$num>>::on_create_list_arc_from_unique(
                         &mut *field
                     )
                 };

(maybe the `;` should be inside the `unsafe` block in this case?)

-- 
Cheers,
Benno

> +            }
> +            unsafe fn on_drop_list_arc(&self) {
> +                let me = self as *const Self;
> +                let field: *const $fty = unsafe { ::core::ptr::addr_of!((*me).$field) };
> +                unsafe { <$fty as $crate::list::ListArcSafe<$num>>::on_drop_list_arc(&*field) };
> +            }
> +        }
> +        unsafe impl$(<$($generics)*>)? TryNewListArc<$num> for $t
> +        where
> +            $fty: TryNewListArc<$num>,
> +        {
> +            fn try_new_list_arc(&self) -> bool {
> +                let me = self as *const Self;
> +                let field: *const $fty = unsafe { ::core::ptr::addr_of!((*me).$field) };
> +                unsafe { <$fty as $crate::list::TryNewListArc<$num>>::try_new_list_arc(&*field) }
> +            }
> +        }
> +        $crate::list::impl_list_arc_safe! { $($rest)* }
> +    };
> +
>      () => {};
>  }
>  pub use impl_list_arc_safe;