linux-kernel - Re: [PATCH] rust: rbtree: add immutable cursor

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] [day] [month] [year] [list]

Message-ID: <CAH5fLgi2UjihwmvdMqBMe0chNpA+OGex2=kLg2NWcXsKmD7wpA@mail.gmail.com>
Date: Fri, 5 Sep 2025 21:43:56 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Vitaly Wool <vitaly.wool@...sulko.se>
Cc: rust-for-linux <rust-for-linux@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, 
	Danilo Krummrich <dakr@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Bjorn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <lossin@...nel.org>, 
	Andreas Hindborg <a.hindborg@...nel.org>, Trevor Gross <tmgross@...ch.edu>, 
	Onur Özkan <work@...rozkan.dev>
Subject: Re: [PATCH] rust: rbtree: add immutable cursor

On Fri, Sep 5, 2025 at 7:30 PM Vitaly Wool <vitaly.wool@...sulko.se> wrote:
>
>
>
> > On Sep 5, 2025, at 11:00 AM, Alice Ryhl <aliceryhl@...gle.com> wrote:
> >
> > On Thu, Sep 04, 2025 at 04:25:52PM +0200, Vitaly Wool wrote:
> >> Sometimes we may need to iterate over, or find an element in a read
> >> only (or read mostly) red-black tree, and in that case we don't need a
> >> mutable reference to the tree, which we'll however have to take to be
> >> able to use the current (mutable) cursor implementation.
> >>
> >> This patch adds a simple immutable cursor implementation to RBTree,
> >> which enables us to use an immutable tree reference. The existing
> >> (fully featured) cursor implementation is renamed to CursorMut,
> >> while retaining its functionality.
> >>
> >> Signed-off-by: Vitaly Wool <vitaly.wool@...sulko.se>
> >
> > Overall looks good to me!
> >
> > A few comments below:
> >
> >> -// SAFETY: The [`Cursor`] has exclusive access to both `K` and `V`, so it is sufficient to require them to be `Send`.
> >> -// The cursor only gives out immutable references to the keys, but since it has excusive access to those same
> >> -// keys, `Send` is sufficient. `Sync` would be okay, but it is more restrictive to the user.
> >> -unsafe impl<'a, K: Send, V: Send> Send for Cursor<'a, K, V> {}
> >> +// SAFETY: The [`CursorMut`] has exclusive access to both `K` and `V`, so it is sufficient to
> >> +// require them to be `Send`.
> >> +// The cursor only gives out immutable references to the keys, but since it has excusive access to
> >> +// those same keys, `Send` is sufficient. `Sync` would be okay, but it is more restrictive to the
> >> +// user.
> >> +unsafe impl<'a, K: Send, V: Send> Send for CursorMut<'a, K, V> {}
> >>
> >> -// SAFETY: The [`Cursor`] gives out immutable references to K and mutable references to V,
> >> +// SAFETY: The [`CursorMut`] gives out immutable references to K and mutable references to V,
> >> // so it has the same thread safety requirements as mutable references.
> >> -unsafe impl<'a, K: Sync, V: Sync> Sync for Cursor<'a, K, V> {}
> >> +unsafe impl<'a, K: Sync, V: Sync> Sync for CursorMut<'a, K, V> {}
> >
> > We should also have Send/Sync impls for the new immutable Cursor type.
> > They need to look like this since the immutable cursor only has shared
> > access and not exclusive access.
> >
> > unsafe impl<'a, K: Sync, V: Sync> Send for Cursor<'a, K, V> {}
> > unsafe impl<'a, K: Sync, V: Sync> Sync for Cursor<'a, K, V> {}
> >
> >> +    /// # Safety
> >> +    ///
> >> +    /// - `node` must be a valid pointer to a node in an [`RBTree`].
> >> +    /// - The caller has immutable access to `node` for the duration of `'b`.
> >> +    unsafe fn to_key_value<'b>(node: NonNull<bindings::rb_node>) -> (&'b K, &'b V) {
> >> +        // SAFETY: the caller guarantees that `node` is a valid pointer in an `RBTree`.
> >> +        let (k, v) = unsafe { Self::to_key_value_raw(node) };
> >> +        // SAFETY: the caller guarantees immutable access to `node`.
> >> +        (k, unsafe { &*v })
> >> +    }
> >> +
> >> +    /// # Safety
> >> +    ///
> >> +    /// - `node` must be a valid pointer to a node in an [`RBTree`].
> >> +    /// - The caller has immutable access to the key for the duration of `'b`.
> >> +    unsafe fn to_key_value_raw<'b>(node: NonNull<bindings::rb_node>) -> (&'b K, *mut V) {
> >
> > The mutable cursor has `to_key_value_raw` because it needs both a &V and
> > `&mut V` version, but the immutable Cursor doesn't have that so it
> > doesn't need a raw version either.
> >
> > That said, perhaps we could share this logic between the two cursor
> > types? We can make these methods standalone instead of being part of the
> > cursor so that both cursors can use the same helper methods instead of
> > duplicating them.
> >
>
> I was thinking about doing some paste macro magic, but maybe it’s better to go the way Boqun is suggesting in another reply and introduce a helper function returning Option<NonNull<..>>. What would you prefer?

I would prefer a helper function.

Alice