Simplify the implementation by removing the closure-based API from
`Guard::load` in favor of returning `Option<NonNull<c_void>>` directly.

Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
 rust/kernel/xarray.rs | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
index 0f69a523b72bf..ca97134ba2bd0 100644
--- a/rust/kernel/xarray.rs
+++ b/rust/kernel/xarray.rs
@@ -211,16 +211,12 @@ fn from(value: StoreError<T>) -> Self {
 }
 
 impl<'a, T: ForeignOwnable> Guard<'a, T> {
-    fn load<F, U>(&self, index: usize, f: F) -> Option<U>
-    where
-        F: FnOnce(NonNull<c_void>) -> U,
-    {
+    fn load(&self, index: usize) -> Option<NonNull<c_void>> {
         let mut state = XArrayState::new(self, index);
         // SAFETY: `state.state` is always valid by the type invariant of
         // `XArrayState`.
         let ptr = unsafe { bindings::xas_load(&raw mut state.state) };
-        let ptr = NonNull::new(ptr.cast())?;
-        Some(f(ptr))
+        NonNull::new(ptr)
     }
 
     /// Checks if the XArray contains an element at the specified index.
@@ -246,18 +242,17 @@ pub fn contains_index(&self, index: usize) -> bool {
 
     /// Provides a reference to the element at the given index.
     pub fn get(&self, index: usize) -> Option<T::Borrowed<'_>> {
-        self.load(index, |ptr| {
-            // SAFETY: `ptr` came from `T::into_foreign`.
-            unsafe { T::borrow(ptr.as_ptr()) }
-        })
+        let ptr = self.load(index)?;
+        // SAFETY: `ptr` came from `T::into_foreign`.
+        Some(unsafe { T::borrow(ptr.as_ptr()) })
     }
 
     /// Provides a mutable reference to the element at the given index.
     pub fn get_mut(&mut self, index: usize) -> Option<T::BorrowedMut<'_>> {
-        self.load(index, |ptr| {
-            // SAFETY: `ptr` came from `T::into_foreign`.
-            unsafe { T::borrow_mut(ptr.as_ptr()) }
-        })
+        let ptr = self.load(index)?;
+
+        // SAFETY: `ptr` came from `T::into_foreign`.
+        Some(unsafe { T::borrow_mut(ptr.as_ptr()) })
     }
 
     /// Removes and returns the element at the given index.

-- 
2.51.2

On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Simplify the implementation by removing the closure-based API from
> `Guard::load` in favor of returning `Option<NonNull<c_void>>` directly.

This is not sound. The returned pointer can now outlive the guard and
mutation through that pointer is trivial.

>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
> ---
>  rust/kernel/xarray.rs | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> index 0f69a523b72bf..ca97134ba2bd0 100644
> --- a/rust/kernel/xarray.rs
> +++ b/rust/kernel/xarray.rs
> @@ -211,16 +211,12 @@ fn from(value: StoreError<T>) -> Self {
>  }
>
>  impl<'a, T: ForeignOwnable> Guard<'a, T> {
> -    fn load<F, U>(&self, index: usize, f: F) -> Option<U>
> -    where
> -        F: FnOnce(NonNull<c_void>) -> U,
> -    {
> +    fn load(&self, index: usize) -> Option<NonNull<c_void>> {
>          let mut state = XArrayState::new(self, index);
>          // SAFETY: `state.state` is always valid by the type invariant of
>          // `XArrayState`.
>          let ptr = unsafe { bindings::xas_load(&raw mut state.state) };
> -        let ptr = NonNull::new(ptr.cast())?;
> -        Some(f(ptr))
> +        NonNull::new(ptr)
>      }
>
>      /// Checks if the XArray contains an element at the specified index.
> @@ -246,18 +242,17 @@ pub fn contains_index(&self, index: usize) -> bool {
>
>      /// Provides a reference to the element at the given index.
>      pub fn get(&self, index: usize) -> Option<T::Borrowed<'_>> {
> -        self.load(index, |ptr| {
> -            // SAFETY: `ptr` came from `T::into_foreign`.
> -            unsafe { T::borrow(ptr.as_ptr()) }
> -        })
> +        let ptr = self.load(index)?;
> +        // SAFETY: `ptr` came from `T::into_foreign`.
> +        Some(unsafe { T::borrow(ptr.as_ptr()) })
>      }
>
>      /// Provides a mutable reference to the element at the given index.
>      pub fn get_mut(&mut self, index: usize) -> Option<T::BorrowedMut<'_>> {
> -        self.load(index, |ptr| {
> -            // SAFETY: `ptr` came from `T::into_foreign`.
> -            unsafe { T::borrow_mut(ptr.as_ptr()) }
> -        })
> +        let ptr = self.load(index)?;
> +
> +        // SAFETY: `ptr` came from `T::into_foreign`.
> +        Some(unsafe { T::borrow_mut(ptr.as_ptr()) })
>      }
>
>      /// Removes and returns the element at the given index.
>
> --
> 2.51.2
>
>

Tamir Duberstein <tamird@gmail.com> writes:

> On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Simplify the implementation by removing the closure-based API from
>> `Guard::load` in favor of returning `Option<NonNull<c_void>>` directly.
>
> This is not sound. The returned pointer can now outlive the guard and
> mutation through that pointer is trivial.

I don't think this is unsound. If we returned a reference instead, it
would be, but we are returning a raw pointer. Dereferencing the pointer
is unsafe and requires proper safety comments.


Best regards,
Andreas Hindborg

On Wed, Jan 7, 2026 at 2:37 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Tamir Duberstein <tamird@gmail.com> writes:
>
> > On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> Simplify the implementation by removing the closure-based API from
> >> `Guard::load` in favor of returning `Option<NonNull<c_void>>` directly.
> >
> > This is not sound. The returned pointer can now outlive the guard and
> > mutation through that pointer is trivial.
>
> I don't think this is unsound. If we returned a reference instead, it
> would be, but we are returning a raw pointer. Dereferencing the pointer
> is unsafe and requires proper safety comments.

You may be right, strictly speaking, but it is most definitely a
footgun. This is a special pointer that requires more careful handling
than other raw pointers.

> Best regards,
> Andreas Hindborg

Tamir Duberstein <tamird@gmail.com> writes:

> On Wed, Jan 7, 2026 at 2:37 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Tamir Duberstein <tamird@gmail.com> writes:
>>
>> > On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>
>> >> Simplify the implementation by removing the closure-based API from
>> >> `Guard::load` in favor of returning `Option<NonNull<c_void>>` directly.
>> >
>> > This is not sound. The returned pointer can now outlive the guard and
>> > mutation through that pointer is trivial.
>>
>> I don't think this is unsound. If we returned a reference instead, it
>> would be, but we are returning a raw pointer. Dereferencing the pointer
>> is unsafe and requires proper safety comments.
>
> You may be right, strictly speaking, but it is most definitely a
> footgun. This is a special pointer that requires more careful handling
> than other raw pointers.

I would disagree. Dereferencing any raw pointer requires the same
checks, and knowing this one is valid and satisfies lifetime
requirements is no different than others.

It is also a private method that is only used in this particular impl
block.

At any rate, I'm fine with dropping the change. The reason I did it was
because I had to double take when I read the previous code. I think the
original code is has some unnecessary complexity with the closure.

Best regards,
Andreas Hindborg

On Thu, Jan 8, 2026 at 4:38 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Tamir Duberstein <tamird@gmail.com> writes:
>
> > On Wed, Jan 7, 2026 at 2:37 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>
> >> Tamir Duberstein <tamird@gmail.com> writes:
> >>
> >> > On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >> >>
> >> >> Simplify the implementation by removing the closure-based API from
> >> >> `Guard::load` in favor of returning `Option<NonNull<c_void>>` directly.
> >> >
> >> > This is not sound. The returned pointer can now outlive the guard and
> >> > mutation through that pointer is trivial.
> >>
> >> I don't think this is unsound. If we returned a reference instead, it
> >> would be, but we are returning a raw pointer. Dereferencing the pointer
> >> is unsafe and requires proper safety comments.
> >
> > You may be right, strictly speaking, but it is most definitely a
> > footgun. This is a special pointer that requires more careful handling
> > than other raw pointers.
>
> I would disagree. Dereferencing any raw pointer requires the same
> checks, and knowing this one is valid and satisfies lifetime
> requirements is no different than others.
>
> It is also a private method that is only used in this particular impl
> block.
>
> At any rate, I'm fine with dropping the change. The reason I did it was
> because I had to double take when I read the previous code. I think the
> original code is has some unnecessary complexity with the closure.

Simplification is always welcome. The current shape of this code was
guided by the desire to avoid the footgun we're discussing here.

>
> Best regards,
> Andreas Hindborg
>
>
>

On Thu Jan 8, 2026 at 11:07 AM GMT, Tamir Duberstein wrote:
> On Thu, Jan 8, 2026 at 4:38 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Tamir Duberstein <tamird@gmail.com> writes:
>>
>> > On Wed, Jan 7, 2026 at 2:37 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >>
>> >> Tamir Duberstein <tamird@gmail.com> writes:
>> >>
>> >> > On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>> >> >>
>> >> >> Simplify the implementation by removing the closure-based API from
>> >> >> `Guard::load` in favor of returning `Option<NonNull<c_void>>` directly.
>> >> >
>> >> > This is not sound. The returned pointer can now outlive the guard and
>> >> > mutation through that pointer is trivial.
>> >>
>> >> I don't think this is unsound. If we returned a reference instead, it
>> >> would be, but we are returning a raw pointer. Dereferencing the pointer
>> >> is unsafe and requires proper safety comments.
>> >
>> > You may be right, strictly speaking, but it is most definitely a
>> > footgun. This is a special pointer that requires more careful handling
>> > than other raw pointers.
>>
>> I would disagree. Dereferencing any raw pointer requires the same
>> checks, and knowing this one is valid and satisfies lifetime
>> requirements is no different than others.
>>
>> It is also a private method that is only used in this particular impl
>> block.
>>
>> At any rate, I'm fine with dropping the change. The reason I did it was
>> because I had to double take when I read the previous code. I think the
>> original code is has some unnecessary complexity with the closure.
>
> Simplification is always welcome. The current shape of this code was
> guided by the desire to avoid the footgun we're discussing here.

Using a closure isn't any better than just returning a raw pointer. If you want
a pointer you can just have a identity closure and there's nothing preventing
you from doing that.

Even with `T::borrow` inside the closure this isn't providing any security as
the code would still compile if you change `'_` in the return signature into
`'static`.

The only defence here is really the `unsafe` and safety comment.

I would prefer to not have closure in this case.

Best,
Gary