| 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
| ||
|
Message-ID: <9ac0aeb3-0f27-4345-8faf-4aeb22f451c5@proton.me>
Date: Tue, 20 Aug 2024 08:00:40 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Matt Gilbride <mattgilbride@...gle.com>, Miguel Ojeda <ojeda@...nel.org>, 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>, Alice Ryhl <aliceryhl@...gle.com>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Arve Hjønnevåg <arve@...roid.com>, Todd Kjos <tkjos@...roid.com>, Martijn Coenen <maco@...roid.com>, Joel Fernandes <joel@...lfernandes.org>, Carlos Llamas <cmllamas@...gle.com>, Suren Baghdasaryan <surenb@...gle.com>, Christian Brauner <brauner@...nel.org>
Cc: Rob Landley <rob@...dley.net>, Davidlohr Bueso <dave@...olabs.net>, Michel Lespinasse <michel@...pinasse.org>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v10 4/5] rust: rbtree: add cursor
On 19.08.24 17:07, Matt Gilbride wrote:
> Add a cursor interface to `RBTree`, supporting the following use cases:
> - Inspect the current node pointed to by the cursor, inspect/move to
> it's neighbors in sort order (bidirectionally).
> - Mutate the tree itself by removing the current node pointed to by the
> cursor, or one of its neighbors.
>
> Add functions to obtain a cursor to the tree by key:
> - The node with the smallest key
> - The node with the largest key
> - The node matching the given key, or the one with the next larger key
>
> The cursor abstraction is needed by the binder driver to efficiently
> search for nodes and (conditionally) modify them, as well as their
> neighbors [1].
>
> Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-6-08ba9197f637@google.com/ [1]
> Co-developed-by: Alice Ryhl <aliceryhl@...gle.com>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> Tested-by: Alice Ryhl <aliceryhl@...gle.com>
> Reviewed-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Matt Gilbride <mattgilbride@...gle.com>
> ---
> rust/kernel/rbtree.rs | 541 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 541 insertions(+)
>
> diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
> index 6c9a8f0a00e6..754af0db86b5 100644
> --- a/rust/kernel/rbtree.rs
> +++ b/rust/kernel/rbtree.rs
> @@ -236,6 +236,40 @@ pub fn values(&self) -> impl Iterator<Item = &'_ V> {
> pub fn values_mut(&mut self) -> impl Iterator<Item = &'_ mut V> {
> self.iter_mut().map(|(_, v)| v)
> }
> +
> + /// Returns a cursor over the tree nodes, starting with the smallest key.
> + pub fn cursor_front(&mut self) -> Option<Cursor<'_, K, V>> {
> + let root = addr_of_mut!(self.root);
> + // SAFETY: `self.root` is always a valid root node
> + let current = unsafe { bindings::rb_first(root) };
> + NonNull::new(current).map(|current| {
> + // INVARIANT:
> + // - `current` is a valid node in the [`RBTree`] pointed to by `self`.
> + // - Due to the type signature of this function, the returned [`Cursor`]
> + // borrows mutably from `self`.
This second invariant doesn't exist on `Cursor`, probably some stale
comment.
> + Cursor {
> + current,
> + tree: self,
> + }
> + })
> + }
> +
> + /// Returns a cursor over the tree nodes, starting with the largest key.
> + pub fn cursor_back(&mut self) -> Option<Cursor<'_, K, V>> {
> + let root = addr_of_mut!(self.root);
> + // SAFETY: `self.root` is always a valid root node
> + let current = unsafe { bindings::rb_last(root) };
> + NonNull::new(current).map(|current| {
> + // INVARIANT:
> + // - `current` is a valid node in the [`RBTree`] pointed to by `self`.
> + // - Due to the type signature of this function, the returned [`Cursor`]
> + // borrows mutably from `self`.
Ditto.
> + Cursor {
> + current,
> + tree: self,
> + }
> + })
> + }
> }
>
> impl<K, V> RBTree<K, V>
> @@ -396,6 +430,75 @@ fn remove_node(&mut self, key: &K) -> Option<RBTreeNode<K, V>> {
> pub fn remove(&mut self, key: &K) -> Option<V> {
> self.remove_node(key).map(|node| node.node.value)
> }
> +
> + /// Returns a cursor over the tree nodes based on the given key.
> + ///
> + /// If the given key exists, the cursor starts there.
> + /// Otherwise it starts with the first larger key in sort order.
> + /// If there is no larger key, it returns [`None`].
> + pub fn cursor_lower_bound(&mut self, key: &K) -> Option<Cursor<'_, K, V>>
> + where
> + K: Ord,
> + {
> + let mut node = self.root.rb_node;
> + let mut best_match: Option<NonNull<Node<K, V>>> = None;
> + while !node.is_null() {
> + // SAFETY: By the type invariant of `Self`, all non-null `rb_node` pointers stored in `self`
> + // point to the links field of `Node<K, V>` objects.
> + let this = unsafe { container_of!(node, Node<K, V>, links) }.cast_mut();
> + // SAFETY: `this` is a non-null node so it is valid by the type invariants.
> + let this_key = unsafe { &(*this).key };
> + // SAFETY: `node` is a non-null node so it is valid by the type invariants.
> + let left_child = unsafe { (*node).rb_left };
> + // SAFETY: `node` is a non-null node so it is valid by the type invariants.
> + let right_child = unsafe { (*node).rb_right };
> + if key == this_key {
> + return NonNull::new(node).map(|current| {
> + // INVARIANT:
> + // - `node` is a valid node in the [`RBTree`] pointed to by `self`.
> + // - Due to the type signature of this function, the returned [`Cursor`]
> + // borrows mutably from `self`.
Here again the stale invariant.
> + Cursor {
> + current,
> + tree: self,
> + }
> + });
You could do this instead of returning here:
best_match = NonNull::new(this);
break;
Then we don't have two places constructing a `Cursor` and can avoid
having to duplicate the `INVARIANT` comment.
> + } else {
> + node = if key > this_key {
I don't know how much this cares about performance, but you can avoid
having to do two comparisons by doing `match key.cmp(this_key)`. Maybe
the compiler is already doing that optimization though.
> + right_child
> + } else {
> + let is_better_match = match best_match {
> + None => true,
> + Some(best) => {
> + // SAFETY: `best` is a non-null node so it is valid by the type invariants.
> + let best_key = unsafe { &(*best.as_ptr()).key };
> + best_key > this_key
> + }
> + };
> + if is_better_match {
> + best_match = NonNull::new(this);
> + }
> + left_child
> + };
> + }
> + }
> +
> + let best = best_match?;
> +
> + // SAFETY: `best` is a non-null node so it is valid by the type invariants.
> + let links = unsafe { addr_of_mut!((*best.as_ptr()).links) };
> +
> + NonNull::new(links).map(|current| {
> + // INVARIANT:
> + // - `current` is a valid node in the [`RBTree`] pointed to by `self`.
> + // - Due to the type signature of this function, the returned [`Cursor`]
> + // borrows mutably from `self`.
Stale invariant (more instances below).
---
Cheers,
Benno
> + Cursor {
> + current,
> + tree: self,
> + }
> + })
> + }
> }
>
> impl<K, V> Default for RBTree<K, V> {
Powered by blists - more mailing lists