linux-kernel - Re: [PATCH v11 2/2] rust: xarray: Add an abstraction for XArray

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: <CAJ-ks9=oyLSkqAsAkO5VSM9js2G2AFvvrA-qHRKNYnsZyUx=mA@mail.gmail.com>
Date: Wed, 11 Dec 2024 10:41:05 -0500
From: Tamir Duberstein <tamird@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Danilo Krummrich <dakr@...nel.org>, Miguel Ojeda <ojeda@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, 
	Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Trevor Gross <tmgross@...ch.edu>, Maíra Canal <mcanal@...lia.com>, 
	Asahi Lina <lina@...hilina.net>, rust-for-linux@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 2/2] rust: xarray: Add an abstraction for XArray

On Wed, Dec 11, 2024 at 10:04 AM Alice Ryhl <aliceryhl@...gle.com> wrote:
>
> > +    fn iter(&self) -> impl Iterator<Item = core::ptr::NonNull<T::PointedTo>> + '_ {
> > +        // TODO: Remove when https://lore.kernel.org/all/20240913213041.395655-5-gary@garyguo.net/ is applied.
> > +        const MAX: core::ffi::c_ulong = core::ffi::c_ulong::MAX;
>
> I think you can use kernel::ffi::c_ulong already. Enough things were
> merged in 6.13 for that to work. If you import kernel::ffi::c_ulong at
> the top of this file, then you can just do c_ulong::MAX in the
> function calls below.

This isn't about using kernel::ffi::c_ulong; it's about using
usize::MAX. I'll clarify the comment and change this to use
kernel::ffi::c_ulong for now.

> > +        let mut index = 0;
> > +
> > +        // SAFETY: `self.xa` is always valid by the type invariant.
> > +        iter::once(unsafe {
> > +            bindings::xa_find(self.xa.get(), &mut index, MAX, bindings::XA_PRESENT)
> > +        })
> > +        .chain(iter::from_fn(move || {
> > +            // SAFETY: `self.xa` is always valid by the type invariant.
> > +            Some(unsafe {
> > +                bindings::xa_find_after(self.xa.get(), &mut index, MAX, bindings::XA_PRESENT)
> > +            })
> > +        }))
> > +        .map_while(|ptr| core::ptr::NonNull::new(ptr.cast()))
>
> You use core::ptr::NonNull in many places. Consider importing it.

Will do.

> > +    /// Stores an entry in the array.
> > +    ///
> > +    /// May drop the lock if needed to allocate memory, and then reacquire it afterwards.
> > +    ///
> > +    /// On success, returns the entry which was previously at the given index.
> > +    ///
> > +    /// On failure, returns the entry which was attempted to be stored.
> > +    pub fn store(
> > +        &mut self,
> > +        index: usize,
> > +        value: T,
> > +        gfp: alloc::Flags,
> > +    ) -> Result<Option<T>, (T, Error)> {
>
> We can see in your examples that this return type is inconvenient.
> Perhaps it would be better to make a new error type containing a T and
> an Error, and implement From so that the question mark can convert
> directly to Error (throwing away the T).

Will do.

> > +// SAFETY: It is safe to send `XArray<T>` to another thread when the underlying `T` is `Send`
> > +// because XArray is thread-safe and all mutation operations are synchronized.
> > +unsafe impl<T: ForeignOwnable + Send> Send for XArray<T> {}
> > +
> > +// SAFETY: It is safe to send `&XArray<T>` to another thread when the underlying `T` is `Sync`
> > +// because it effectively means sharing `&T` (which is safe because `T` is `Sync`); additionally, it
> > +// needs `T` to be `Send` because any thread that has a `&XArray<T>` may lock it and get a
> > +// `Guard<T>` on that thread, so the thread may ultimately access `T` using a mutable reference, for
> > +// example, using `get_mut` or `remove`.
> > +unsafe impl<T: ForeignOwnable + Send + Sync> Sync for XArray<T> {}
>
> I don't think Sync is needed due to the spinlock.

Agreed. How's this phrasing for the comment?

// SAFETY: It is safe to send `&XArray<T>` to another thread when the
underlying `T` is `Send`
// because any thread that has a `&XArray<T>` may lock it and get a
`Guard<T>` on that thread, so
// the thread may ultimately access `T` using a mutable borrow, for
example, using `get_mut` or
// `remove`. It is not necessary for `T` to be `Sync` because access
to immutable borrows of `T` is
// also synchronized through `Guard<T>`.