rbtree: reduce unsafe blocks on pointer derefs

PATCH Applied Onur Özkan 4 participants 9 messages Nov 13, 2025 – Jan 19 Rust

Share & tools

Onur Özkan

author reviewer

2025-11-13 14:53 UTC #0 lore raw

PATCH 1/1 rbtree: reduce unsafe blocks on pointer derefs

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 <[email protected]>

---
 rust/kernel/rbtree.rs | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

rust/kernel/rbtree.rs +15 ~~-12~~

index b8fe6be6fcc4..d44f305e5161 100644
@@ -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;
                 }
             };
         }

Signature · 1 line

2.51.2

Charalampos Mitrodimas

Acked-by on v1 1/1

2025-11-17 12:13 UTC #1 lore raw

Quoted 11 lines · 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.

Quoted 9 lines

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 <[email protected]>
---
 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 <[email protected]>

--
C. Mitrodimas

Miguel Ojeda

2026-01-18 21:03 UTC #2 lore raw

Quoted 4 lines · Charalampos Mitrodimas

This is a nice change to have if it reaches the general consensus.
Acked-by Charalampos Mitrodimas <[email protected]>

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

Charalampos Mitrodimas

2026-01-19 08:37 UTC #3 lore raw

Quoted 7 lines · Miguel Ojeda

Quoted 4 lines · Charalampos Mitrodimas
This is a nice change to have if it reaches the general consensus.
Acked-by Charalampos Mitrodimas <[email protected]>

Quoted 7 lines

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.

Quoted 2 lines

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

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

C. Mitrodimas

Quoted 3 lines

Cheers,
Miguel

Miguel Ojeda

2026-01-19 08:40 UTC #4 lore raw

Quoted 3 lines · Charalampos Mitrodimas

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

Alice Ryhl

reviewer Reviewed-by on v1 1/1

2026-01-14 08:42 UTC #5 lore raw

In reply to Onur Özkan

Quoted 10 lines · 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 }),

Quoted 7 lines

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 <[email protected]>

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 <[email protected]>

Miguel Ojeda

2026-01-18 21:02 UTC #6 lore raw

In reply to Onur Özkan

Quoted 2 lines · Onur Özkan

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

Alice Ryhl

reviewer

2026-01-18 21:27 UTC #7 lore raw

Quoted 10 lines · Miguel Ojeda

Quoted 2 lines · Onur Özkan
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

Miguel Ojeda

Applied

2026-01-19 08:21 UTC #8 lore raw

In reply to Onur Özkan

Quoted 10 lines · 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 }),

Quoted 7 lines

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 <[email protected]>

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