[PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Onur Özkan]

Refactors parts of the get() and cursor_lower_bound()
traversal logic to minimize the scope of unsafe blocks
and avoid duplicating same safety comments.

One of the removed comments was also misleading:

    // SAFETY: `node` is a non-null node...
    Ordering::Equal => return Some(unsafe { &(*this).value }),

as `node` should have been `this`.

No functional changes intended; this is purely a safety
improvement that reduces the amount of unsafe blocks
while keeping all invariants intact.

Signed-off-by: Onur Özkan <work@onurozkan.dev>
---
 rust/kernel/rbtree.rs | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/rust/kernel/rbtree.rs b/rust/kernel/rbtree.rs
index b8fe6be6fcc4..d44f305e5161 100644
--- a/rust/kernel/rbtree.rs
+++ b/rust/kernel/rbtree.rs
@@ -384,14 +384,17 @@ pub fn get(&self, key: &K) -> Option<&V> {
             // 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) };
+
             // SAFETY: `this` is a non-null node so it is valid by the type invariants.
-            node = match key.cmp(unsafe { &(*this).key }) {
-                // SAFETY: `node` is a non-null node so it is valid by the type invariants.
-                Ordering::Less => unsafe { (*node).rb_left },
-                // SAFETY: `node` is a non-null node so it is valid by the type invariants.
-                Ordering::Greater => unsafe { (*node).rb_right },
-                // SAFETY: `node` is a non-null node so it is valid by the type invariants.
-                Ordering::Equal => return Some(unsafe { &(*this).value }),
+            let this_ref = unsafe { &*this };
+
+            // SAFETY: `node` is a non-null node so it is valid by the type invariants.
+            let node_ref = unsafe { &*node };
+
+            node = match key.cmp(&this_ref.key) {
+                Ordering::Less => node_ref.rb_left,
+                Ordering::Greater => node_ref.rb_right,
+                Ordering::Equal => return Some(&this_ref.value),
             }
         }
         None
@@ -433,17 +436,17 @@ pub fn cursor_lower_bound(&mut self, key: &K) -> Option<Cursor<'_, K, V>>
             let this = unsafe { container_of!(node, Node<K, V>, links) };
             // 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 };
+            let node_ref = unsafe { &*node };
+
             match key.cmp(this_key) {
                 Ordering::Equal => {
                     best_match = NonNull::new(this);
                     break;
                 }
                 Ordering::Greater => {
-                    node = right_child;
+                    node = node_ref.rb_right;
                 }
                 Ordering::Less => {
                     let is_better_match = match best_match {
@@ -457,7 +460,7 @@ pub fn cursor_lower_bound(&mut self, key: &K) -> Option<Cursor<'_, K, V>>
                     if is_better_match {
                         best_match = NonNull::new(this);
                     }
-                    node = left_child;
+                    node = node_ref.rb_left;
                 }
             };
         }
-- 
2.51.2

Re: [PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Charalampos Mitrodimas]

Onur Özkan <work@onurozkan.dev> writes:

> Refactors parts of the get() and cursor_lower_bound()
> traversal logic to minimize the scope of unsafe blocks
> and avoid duplicating same safety comments.
>
> One of the removed comments was also misleading:
>
>     // SAFETY: `node` is a non-null node...
>     Ordering::Equal => return Some(unsafe { &(*this).value }),
>
> as `node` should have been `this`.
>
> No functional changes intended; this is purely a safety
> improvement that reduces the amount of unsafe blocks
> while keeping all invariants intact.
>
> Signed-off-by: Onur Özkan <work@onurozkan.dev>
> ---
>  rust/kernel/rbtree.rs | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>

This is a nice change to have if it reaches the general consensus.

Acked-By: Charalampos Mitrodimas <charmitro@posteo.net>

--
C. Mitrodimas

Re: [PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Alice Ryhl]

On Thu, Nov 13, 2025 at 3:46 PM Onur Özkan <work@onurozkan.dev> wrote:
>
> Refactors parts of the get() and cursor_lower_bound()
> traversal logic to minimize the scope of unsafe blocks
> and avoid duplicating same safety comments.
>
> One of the removed comments was also misleading:
>
>     // SAFETY: `node` is a non-null node...
>     Ordering::Equal => return Some(unsafe { &(*this).value }),
>
> as `node` should have been `this`.
>
> No functional changes intended; this is purely a safety
> improvement that reduces the amount of unsafe blocks
> while keeping all invariants intact.
>
> Signed-off-by: Onur Özkan <work@onurozkan.dev>

One consequence of creating a &_ to the bindings::rb_node struct means
that we assert immutability for the entire struct and not just the
rb_left/rb_right fields, but I have verified that this is ok.

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Miguel Ojeda]

On Thu, Nov 13, 2025 at 3:46 PM Onur Özkan <work@onurozkan.dev> wrote:
>
> Refactors parts of the get() and cursor_lower_bound()

This is now `find_best_match()` given commit f56b13172382 ("rust:
rbtree: add immutable cursor").

Alice: I assume your analysis/review still applies even after Vitaly's
refactor -- if not, please shout!

Cheers,
Miguel

Re: [PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Miguel Ojeda]

On Mon, Nov 17, 2025 at 1:13 PM Charalampos Mitrodimas
<charmitro@posteo.net> wrote:
>
> This is a nice change to have if it reaches the general consensus.
>
> Acked-By: Charalampos Mitrodimas <charmitro@posteo.net>

So, typically, Acked-by tags are given by maintainers of the code.
Sometimes, other stakeholders give it too:

  https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

I will keep the tag, but if this was meant to be a Reviewed-by or
similar instead, please let me know.

(Nit: it is usually -by, not -By).

Cheers,
Miguel

Re: [PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Alice Ryhl]

On Sun, Jan 18, 2026 at 10:02 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Thu, Nov 13, 2025 at 3:46 PM Onur Özkan <work@onurozkan.dev> wrote:
> >
> > Refactors parts of the get() and cursor_lower_bound()
>
> This is now `find_best_match()` given commit f56b13172382 ("rust:
> rbtree: add immutable cursor").
>
> Alice: I assume your analysis/review still applies even after Vitaly's
> refactor -- if not, please shout!

It's still fine, thanks.

Alice

Re: [PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Miguel Ojeda]

On Thu, Nov 13, 2025 at 3:46 PM Onur Özkan <work@onurozkan.dev> wrote:
>
> Refactors parts of the get() and cursor_lower_bound()
> traversal logic to minimize the scope of unsafe blocks
> and avoid duplicating same safety comments.
>
> One of the removed comments was also misleading:
>
>     // SAFETY: `node` is a non-null node...
>     Ordering::Equal => return Some(unsafe { &(*this).value }),
>
> as `node` should have been `this`.
>
> No functional changes intended; this is purely a safety
> improvement that reduces the amount of unsafe blocks
> while keeping all invariants intact.
>
> Signed-off-by: Onur Özkan <work@onurozkan.dev>

Applied to `rust-next` -- thanks everyone!

    [ Alice writes:

        "One consequence of creating a &_ to the bindings::rb_node struct means
        that we assert immutability for the entire struct and not just the
        rb_left/rb_right fields, but I have verified that this is ok."

          - Miguel ]

    [ Reworded title and replaced `cursor_lower_bound()` with
      `find_best_match()` in message. - Miguel ]

Cheers,
Miguel

Re: [PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Charalampos Mitrodimas]

Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:

> On Mon, Nov 17, 2025 at 1:13 PM Charalampos Mitrodimas
> <charmitro@posteo.net> wrote:
>>
>> This is a nice change to have if it reaches the general consensus.
>>
>> Acked-By: Charalampos Mitrodimas <charmitro@posteo.net>
>
> So, typically, Acked-by tags are given by maintainers of the code.
> Sometimes, other stakeholders give it too:
>
>   https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
>
> I will keep the tag, but if this was meant to be a Reviewed-by or
> similar instead, please let me know.

Hi Miguel,

I was not aware, thanks for the heads up! Assume it is a Reviewed-by or
just discard since it's invalid.

>
> (Nit: it is usually -by, not -By).

Ah yes, need to update my email client snippet configuration.

C. Mitrodimas

>
> Cheers,
> Miguel

Re: [PATCH v1] rbtree: reduce unsafe blocks on pointer derefs

[Miguel Ojeda]

On Mon, Jan 19, 2026 at 9:36 AM Charalampos Mitrodimas
<charmitro@posteo.net> wrote:
>
> I was not aware, thanks for the heads up! Assume it is a Reviewed-by or
> just discard since it's invalid.

Ah, great, thanks for the quick reply -- updated to Reviewed-by in the
commit then.

Cheers,
Miguel