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

Message-Id: <F53AA863-91B7-459E-98B2-74FA44BA48AB@konsulko.se>
Date: Fri, 5 Sep 2025 19:30:47 +0200
From: Vitaly Wool <vitaly.wool@...sulko.se>
To: Alice Ryhl <aliceryhl@...gle.com>
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 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?

Best regards,
Vitaly