linux-kernel - Re: [PATCH 04/10] rust: xarray: add `XArrayState`

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: <20260108142239.320ea3b4.gary@garyguo.net>
Date: Thu, 8 Jan 2026 14:22:39 +0000
From: Gary Guo <gary@...yguo.net>
To: Andreas Hindborg <a.hindborg@...nel.org>
Cc: Tamir Duberstein <tamird@...il.com>, Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin
 <lossin@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross
 <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, Daniel Gomez
 <da.gomez@...nel.org>, rust-for-linux@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/10] rust: xarray: add `XArrayState`

On Wed, 07 Jan 2026 19:48:49 +0100
Andreas Hindborg <a.hindborg@...nel.org> wrote:

> Tamir Duberstein <tamird@...il.com> writes:
> 
> > On Wed, Dec 3, 2025 at 5:27 PM Andreas Hindborg <a.hindborg@...nel.org> wrote:  
> >>
> >> Add `XArrayState` as internal state for XArray iteration and entry
> >> operations. This struct wraps the C `xa_state` structure and holds a
> >> reference to a `Guard` to ensure exclusive access to the XArray for the
> >> lifetime of the state object.
> >>
> >> The `XAS_RESTART` constant is also exposed through the bindings helper
> >> to properly initialize the `xa_node` field.
> >>
> >> The struct and its constructor are marked with `#[expect(dead_code)]` as
> >> there are no users yet. We will remove this annotation in a later patch.
> >>
> >> Signed-off-by: Andreas Hindborg <a.hindborg@...nel.org>
> >> ---
> >>  rust/bindings/bindings_helper.h |  1 +
> >>  rust/kernel/xarray.rs           | 41 ++++++++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 41 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> >> index 2e43c66635a2c..86bca946faff0 100644
> >> --- a/rust/bindings/bindings_helper.h
> >> +++ b/rust/bindings/bindings_helper.h
> >> @@ -109,6 +109,7 @@ const xa_mark_t RUST_CONST_HELPER_XA_PRESENT = XA_PRESENT;
> >>  const gfp_t RUST_CONST_HELPER_XA_FLAGS_ALLOC = XA_FLAGS_ALLOC;
> >>  const gfp_t RUST_CONST_HELPER_XA_FLAGS_ALLOC1 = XA_FLAGS_ALLOC1;
> >>  const vm_flags_t RUST_CONST_HELPER_VM_MERGEABLE = VM_MERGEABLE;
> >> +const size_t RUST_CONST_HELPER_XAS_RESTART = (size_t)XAS_RESTART;  
> >
> > Can we avoid casting it here? It's quite distant from the site of use
> > which makes it difficult to understand why it's necessary. It also
> > suffers from C casting rules being quite lax, which means a change in
> > the type on the C side would likely go unnoticed.  
> 
> Yes, we should probably use `*mut bindings::xa_node` for this type, then
> the casts at the use side in Rust will go away as well. I'll try that.

I don't think bindgen can generate a pointer-typed constant. I think what
you did the original patch is correct, as XAS_RESTART is supposed to be an
plain address (without provenance).

Best,
Gary

> 
> >  
> >>
> >>  #if IS_ENABLED(CONFIG_ANDROID_BINDER_IPC_RUST)
> >>  #include "../../drivers/android/binder/rust_binder.h"
> >> diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
> >> index ede48b5e1dba3..f6e610b059625 100644
> >> --- a/rust/kernel/xarray.rs
> >> +++ b/rust/kernel/xarray.rs
> >> @@ -8,7 +8,10 @@
> >>      iter,
> >>      marker::PhantomData,
> >>      pin::Pin,
> >> -    ptr::NonNull, //
> >> +    ptr::{
> >> +        null_mut,
> >> +        NonNull, //
> >> +    },
> >>  };
> >>  use kernel::{
> >>      alloc,
> >> @@ -319,6 +322,42 @@ pub fn store(
> >>      }
> >>  }
> >>
> >> +/// Internal state for XArray iteration and entry operations.
> >> +///
> >> +/// # Invariants
> >> +///
> >> +/// - `state` is always a valid `bindings::xa_state`.
> >> +#[expect(dead_code)]
> >> +pub(crate) struct XArrayState<'a, 'b, T: ForeignOwnable> {
> >> +    /// Holds a reference to the lock guard to ensure the lock is not dropped
> >> +    /// while `Self` is live.
> >> +    _access: &'b Guard<'a, T>,  
> >
> > Can this comment explain why this isn't a PhantomData? IIUC we just
> > need the lifetime, rather than the actual guard.  
> 
> This can be a PhantomData. The use of the reference is leftover from
> earlier prototypes. I'll change it.
> 
> >  
> >> +    state: bindings::xa_state,
> >> +}
> >> +
> >> +impl<'a, 'b, T: ForeignOwnable> XArrayState<'a, 'b, T> {
> >> +    #[expect(dead_code)]
> >> +    fn new(access: &'b Guard<'a, T>, index: usize) -> Self {
> >> +        let ptr = access.xa.xa.get();
> >> +        // INVARIANT: We initialize `self.state` to a valid value below.
> >> +        Self {
> >> +            _access: access,
> >> +            state: bindings::xa_state {
> >> +                xa: ptr,
> >> +                xa_index: index,
> >> +                xa_shift: 0,
> >> +                xa_sibs: 0,
> >> +                xa_offset: 0,
> >> +                xa_pad: 0,
> >> +                xa_node: bindings::XAS_RESTART as *mut bindings::xa_node,  
> >
> > Odd that we're casting it back to a pointer here.  
> 
> Yes, full cycle. I'll try to use the pointer type for the constant.
> 
> Thanks for the comments.
> 
> Best regards,
> Andreas Hindborg
> 
> 
>