[PATCH v15 0/3] rust: xarray: Add a minimal abstraction for XArray

[PATCH v15 0/3] rust: xarray: Add a minimal abstraction for XArray

[Tamir Duberstein]

This is a reimagining relative to earlier versions[0] by Asahi Lina and
Maíra Canal.

It is needed to support rust-binder, though this version only provides
enough machinery to support rnull.

Link: https://lore.kernel.org/rust-for-linux/20240309235927.168915-2-mcanal@igalia.com/ [0]
---
Changes in v15:
- Rebase on v6.14-rc1.
- Add MAINTAINERS entry.
- Link to v14: https://lore.kernel.org/r/20241217-rust-xarray-bindings-v14-0-9fef3cefcb41@gmail.com

Changes in v14:
- Remove TODO made stale by Gary Guo's FFI type series.
- Link: https://lore.kernel.org/all/20240913213041.395655-5-gary@garyguo.net/
- Link to v13: https://lore.kernel.org/r/20241213-rust-xarray-bindings-v13-0-8655164e624f@gmail.com

Changes in v13:
- Replace `bool::then` with `if`. (Miguel Ojeda)
- Replace `match` with `let` + `if`. (Miguel Ojeda)
- Link to v12: https://lore.kernel.org/r/20241212-rust-xarray-bindings-v12-0-59ab9b1f4d2e@gmail.com

Changes in v12:
- Import `core::ptr::NonNull`. (Alice Ryhl)
- Introduce `StoreError` to allow `?` to be used with `Guard::store`.
  (Alice Ryhl)
- Replace `{crate,core}::ffi::c_ulong` and clarify TODO with respect to
  `usize`. (Alice Ryhl)
- Drop `T: Sync` bound on `impl Sync for XArray<T>`. (Alice Ryhl)
- Reword `Send` and `Sync` safety comments to match the style used in
  `lock.rs`. (Alice Ryhl and Andreas
  Hindborg)
- Link to v11: https://lore.kernel.org/r/20241203-rust-xarray-bindings-v11-0-58a95d137ec2@gmail.com

Changes in v11:
- Consolidate imports. (Alice Ryhl)
- Use literal `0` rather than `MIN`. (Alice Ryhl)
- Use bulleted list in SAFETY comment. (Alice Ryhl)
- Document (un)locking behavior of `Guard::store`. (Alice Ryhl)
- Document Normal API behavior WRT `XA_ZERO_ENTRY`. (Alice Ryhl)
- Rewrite `unsafe impl Sync` SAFETY comment. (Andreas Hindborg)
- Link to v10: https://lore.kernel.org/r/20241120-rust-xarray-bindings-v10-0-a25b2b0bf582@gmail.com

Changes in v10:
- Guard::get takes &self instead of &mut self. (Andreas Hindborg)
- Guard is !Send. (Boqun Feng)
- Add Inspired-by tags. (Maíra Canal and Asahi Lina)
- Rebase on linux-next, use NotThreadSafe. (Alice Ryhl)
- Link to v9: https://lore.kernel.org/r/20241118-rust-xarray-bindings-v9-0-3219cdb53685@gmail.com

---
Tamir Duberstein (3):
      rust: types: add `ForeignOwnable::PointedTo`
      rust: xarray: Add an abstraction for XArray
      MAINTAINERS: add entry for Rust XArray API

 MAINTAINERS                     |  11 ++
 rust/bindings/bindings_helper.h |   6 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/xarray.c           |  28 ++++
 rust/kernel/alloc.rs            |   5 +
 rust/kernel/alloc/kbox.rs       |  38 +++---
 rust/kernel/lib.rs              |   1 +
 rust/kernel/miscdevice.rs       |   7 +-
 rust/kernel/pci.rs              |   5 +-
 rust/kernel/platform.rs         |   5 +-
 rust/kernel/sync/arc.rs         |  21 +--
 rust/kernel/types.rs            |  46 ++++---
 rust/kernel/xarray.rs           | 279 ++++++++++++++++++++++++++++++++++++++++
 13 files changed, 408 insertions(+), 45 deletions(-)
---
base-commit: e47e7490144c9fb26590447f73d90538e0934a75
change-id: 20241020-rust-xarray-bindings-bef514142968

Best regards,
-- 
Tamir Duberstein <tamird@gmail.com>

[PATCH v15 1/3] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

Allow implementors to specify the foreign pointer type; this exposes
information about the pointed-to type such as its alignment.

This requires the trait to be `unsafe` since it is now possible for
implementors to break soundness by returning a misaligned pointer.

Encoding the pointer type in the trait (and avoiding pointer casts)
allows the compiler to check that implementors return the correct
pointer type. This is preferable to directly encoding the alignment in
the trait using a constant as the compiler would be unable to check it.

Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
 rust/kernel/miscdevice.rs |  7 ++++++-
 rust/kernel/pci.rs        |  5 ++++-
 rust/kernel/platform.rs   |  5 ++++-
 rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
 rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
 6 files changed, 77 insertions(+), 45 deletions(-)

diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
index cb4ebea3b074..55529832db54 100644
--- a/rust/kernel/alloc/kbox.rs
+++ b/rust/kernel/alloc/kbox.rs
@@ -349,68 +349,70 @@ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
     }
 }
 
-impl<T: 'static, A> ForeignOwnable for Box<T, A>
+// SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
+unsafe impl<T: 'static, A> ForeignOwnable for Box<T, A>
 where
     A: Allocator,
 {
+    type PointedTo = T;
     type Borrowed<'a> = &'a T;
     type BorrowedMut<'a> = &'a mut T;
 
-    fn into_foreign(self) -> *mut crate::ffi::c_void {
-        Box::into_raw(self).cast()
+    fn into_foreign(self) -> *mut Self::PointedTo {
+        Box::into_raw(self)
     }
 
-    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self {
+    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        unsafe { Box::from_raw(ptr.cast()) }
+        unsafe { Box::from_raw(ptr) }
     }
 
-    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> &'a T {
+    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> &'a T {
         // SAFETY: The safety requirements of this method ensure that the object remains alive and
         // immutable for the duration of 'a.
-        unsafe { &*ptr.cast() }
+        unsafe { &*ptr }
     }
 
-    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> &'a mut T {
-        let ptr = ptr.cast();
+    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> &'a mut T {
         // SAFETY: The safety requirements of this method ensure that the pointer is valid and that
         // nothing else will access the value for the duration of 'a.
         unsafe { &mut *ptr }
     }
 }
 
-impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
+// SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
+unsafe impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
 where
     A: Allocator,
 {
+    type PointedTo = T;
     type Borrowed<'a> = Pin<&'a T>;
     type BorrowedMut<'a> = Pin<&'a mut T>;
 
-    fn into_foreign(self) -> *mut crate::ffi::c_void {
+    fn into_foreign(self) -> *mut Self::PointedTo {
         // SAFETY: We are still treating the box as pinned.
-        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }).cast()
+        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) })
     }
 
-    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self {
+    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        unsafe { Pin::new_unchecked(Box::from_raw(ptr.cast())) }
+        unsafe { Pin::new_unchecked(Box::from_raw(ptr)) }
     }
 
-    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> Pin<&'a T> {
+    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Pin<&'a T> {
         // SAFETY: The safety requirements for this function ensure that the object is still alive,
         // so it is safe to dereference the raw pointer.
         // The safety requirements of `from_foreign` also ensure that the object remains alive for
         // the lifetime of the returned value.
-        let r = unsafe { &*ptr.cast() };
+        let r = unsafe { &*ptr };
 
         // SAFETY: This pointer originates from a `Pin<Box<T>>`.
         unsafe { Pin::new_unchecked(r) }
     }
 
-    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> Pin<&'a mut T> {
-        let ptr = ptr.cast();
+    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> Pin<&'a mut T> {
         // SAFETY: The safety requirements for this function ensure that the object is still alive,
         // so it is safe to dereference the raw pointer.
         // The safety requirements of `from_foreign` also ensure that the object remains alive for
diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index e14433b2ab9d..f1a081dd64c7 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -225,13 +225,15 @@ impl<T: MiscDevice> VtableHelper<T> {
         Ok(ptr) => ptr,
         Err(err) => return err.to_errno(),
     };
+    let ptr = ptr.into_foreign();
+    let ptr = ptr.cast();
 
     // This overwrites the private data with the value specified by the user, changing the type of
     // this file's private data. All future accesses to the private data is performed by other
     // fops_* methods in this file, which all correctly cast the private data to the new type.
     //
     // SAFETY: The open call of a file can access the private data.
-    unsafe { (*raw_file).private_data = ptr.into_foreign() };
+    unsafe { (*raw_file).private_data = ptr };
 
     0
 }
@@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> {
 ) -> c_int {
     // SAFETY: The release call of a file owns the private data.
     let private = unsafe { (*file).private_data };
+    let private = private.cast();
     // SAFETY: The release call of a file owns the private data.
     let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
 
@@ -267,6 +270,7 @@ impl<T: MiscDevice> VtableHelper<T> {
 ) -> c_long {
     // SAFETY: The ioctl call of a file can access the private data.
     let private = unsafe { (*file).private_data };
+    let private = private.cast();
     // SAFETY: Ioctl calls can borrow the private data of the file.
     let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
 
@@ -316,6 +320,7 @@ impl<T: MiscDevice> VtableHelper<T> {
 ) {
     // SAFETY: The release call of a file owns the private data.
     let private = unsafe { (*file).private_data };
+    let private = private.cast();
     // SAFETY: Ioctl calls can borrow the private data of the file.
     let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
     // SAFETY:
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 4c98b5b9aa1e..eb25fabbff9c 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -72,10 +72,12 @@ extern "C" fn probe_callback(
 
         match T::probe(&mut pdev, info) {
             Ok(data) => {
+                let data = data.into_foreign();
+                let data = data.cast();
                 // Let the `struct pci_dev` own a reference of the driver's private data.
                 // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
                 // `struct pci_dev`.
-                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
+                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data) };
             }
             Err(err) => return Error::to_errno(err),
         }
@@ -87,6 +89,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
         // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
         // `struct pci_dev`.
         let ptr = unsafe { bindings::pci_get_drvdata(pdev) };
+        let ptr = ptr.cast();
 
         // SAFETY: `remove_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 50e6b0421813..53764cb7f804 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -63,10 +63,12 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
         let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
         match T::probe(&mut pdev, info) {
             Ok(data) => {
+                let data = data.into_foreign();
+                let data = data.cast();
                 // Let the `struct platform_device` own a reference of the driver's private data.
                 // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
                 // `struct platform_device`.
-                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
+                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data) };
             }
             Err(err) => return Error::to_errno(err),
         }
@@ -77,6 +79,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
     extern "C" fn remove_callback(pdev: *mut bindings::platform_device) {
         // SAFETY: `pdev` is a valid pointer to a `struct platform_device`.
         let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
+        let ptr = ptr.cast();
 
         // SAFETY: `remove_callback` is only ever called after a successful call to
         // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs
index 3cefda7a4372..dfe4abf82c25 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -140,9 +140,10 @@ pub struct Arc<T: ?Sized> {
     _p: PhantomData<ArcInner<T>>,
 }
 
+#[doc(hidden)]
 #[pin_data]
 #[repr(C)]
-struct ArcInner<T: ?Sized> {
+pub struct ArcInner<T: ?Sized> {
     refcount: Opaque<bindings::refcount_t>,
     data: T,
 }
@@ -342,18 +343,20 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
     }
 }
 
-impl<T: 'static> ForeignOwnable for Arc<T> {
+// SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
+unsafe impl<T: 'static> ForeignOwnable for Arc<T> {
+    type PointedTo = ArcInner<T>;
     type Borrowed<'a> = ArcBorrow<'a, T>;
     type BorrowedMut<'a> = Self::Borrowed<'a>;
 
-    fn into_foreign(self) -> *mut crate::ffi::c_void {
-        ManuallyDrop::new(self).ptr.as_ptr().cast()
+    fn into_foreign(self) -> *mut Self::PointedTo {
+        ManuallyDrop::new(self).ptr.as_ptr()
     }
 
-    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self {
+    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        let inner = unsafe { NonNull::new_unchecked(ptr.cast::<ArcInner<T>>()) };
+        let inner = unsafe { NonNull::new_unchecked(ptr) };
 
         // SAFETY: By the safety requirement of this function, we know that `ptr` came from
         // a previous call to `Arc::into_foreign`, which guarantees that `ptr` is valid and
@@ -361,17 +364,17 @@ unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self {
         unsafe { Self::from_inner(inner) }
     }
 
-    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> ArcBorrow<'a, T> {
+    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> ArcBorrow<'a, T> {
         // SAFETY: The safety requirements of this function ensure that `ptr` comes from a previous
         // call to `Self::into_foreign`.
-        let inner = unsafe { NonNull::new_unchecked(ptr.cast::<ArcInner<T>>()) };
+        let inner = unsafe { NonNull::new_unchecked(ptr) };
 
         // SAFETY: The safety requirements of `from_foreign` ensure that the object remains alive
         // for the lifetime of the returned value.
         unsafe { ArcBorrow::new(inner) }
     }
 
-    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> ArcBorrow<'a, T> {
+    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> ArcBorrow<'a, T> {
         // SAFETY: The safety requirements for `borrow_mut` are a superset of the safety
         // requirements for `borrow`.
         unsafe { Self::borrow(ptr) }
diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
index 2bbaab83b9d6..55ddd50e8aaa 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -18,7 +18,19 @@
 ///
 /// This trait is meant to be used in cases when Rust objects are stored in C objects and
 /// eventually "freed" back to Rust.
-pub trait ForeignOwnable: Sized {
+///
+/// # Safety
+///
+/// Implementers must ensure that [`into_foreign`] returns a pointer which meets the alignment
+/// requirements of [`PointedTo`].
+///
+/// [`into_foreign`]: Self::into_foreign
+/// [`PointedTo`]: Self::PointedTo
+pub unsafe trait ForeignOwnable: Sized {
+    /// Type used when the value is foreign-owned. In practical terms only defines the alignment of
+    /// the pointer.
+    type PointedTo;
+
     /// Type used to immutably borrow a value that is currently foreign-owned.
     type Borrowed<'a>;
 
@@ -27,16 +39,18 @@ pub trait ForeignOwnable: Sized {
 
     /// Converts a Rust-owned object to a foreign-owned one.
     ///
-    /// The foreign representation is a pointer to void. There are no guarantees for this pointer.
-    /// For example, it might be invalid, dangling or pointing to uninitialized memory. Using it in
-    /// any way except for [`from_foreign`], [`try_from_foreign`], [`borrow`], or [`borrow_mut`] can
-    /// result in undefined behavior.
+    /// # Guarantees
+    ///
+    /// The return value is guaranteed to be well-aligned, but there are no other guarantees for
+    /// this pointer. For example, it might be null, dangling, or point to uninitialized memory.
+    /// Using it in any way except for [`ForeignOwnable::from_foreign`], [`ForeignOwnable::borrow`],
+    /// [`ForeignOwnable::try_from_foreign`] can result in undefined behavior.
     ///
     /// [`from_foreign`]: Self::from_foreign
     /// [`try_from_foreign`]: Self::try_from_foreign
     /// [`borrow`]: Self::borrow
     /// [`borrow_mut`]: Self::borrow_mut
-    fn into_foreign(self) -> *mut crate::ffi::c_void;
+    fn into_foreign(self) -> *mut Self::PointedTo;
 
     /// Converts a foreign-owned object back to a Rust-owned one.
     ///
@@ -46,7 +60,7 @@ pub trait ForeignOwnable: Sized {
     /// must not be passed to `from_foreign` more than once.
     ///
     /// [`into_foreign`]: Self::into_foreign
-    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self;
+    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self;
 
     /// Tries to convert a foreign-owned object back to a Rust-owned one.
     ///
@@ -58,7 +72,7 @@ pub trait ForeignOwnable: Sized {
     /// `ptr` must either be null or satisfy the safety requirements for [`from_foreign`].
     ///
     /// [`from_foreign`]: Self::from_foreign
-    unsafe fn try_from_foreign(ptr: *mut crate::ffi::c_void) -> Option<Self> {
+    unsafe fn try_from_foreign(ptr: *mut Self::PointedTo) -> Option<Self> {
         if ptr.is_null() {
             None
         } else {
@@ -81,7 +95,7 @@ unsafe fn try_from_foreign(ptr: *mut crate::ffi::c_void) -> Option<Self> {
     ///
     /// [`into_foreign`]: Self::into_foreign
     /// [`from_foreign`]: Self::from_foreign
-    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> Self::Borrowed<'a>;
+    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Self::Borrowed<'a>;
 
     /// Borrows a foreign-owned object mutably.
     ///
@@ -109,21 +123,23 @@ unsafe fn try_from_foreign(ptr: *mut crate::ffi::c_void) -> Option<Self> {
     /// [`from_foreign`]: Self::from_foreign
     /// [`borrow`]: Self::borrow
     /// [`Arc`]: crate::sync::Arc
-    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> Self::BorrowedMut<'a>;
+    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> Self::BorrowedMut<'a>;
 }
 
-impl ForeignOwnable for () {
+// SAFETY: The `into_foreign` function returns a pointer that is dangling, but well-aligned.
+unsafe impl ForeignOwnable for () {
+    type PointedTo = ();
     type Borrowed<'a> = ();
     type BorrowedMut<'a> = ();
 
-    fn into_foreign(self) -> *mut crate::ffi::c_void {
+    fn into_foreign(self) -> *mut Self::PointedTo {
         core::ptr::NonNull::dangling().as_ptr()
     }
 
-    unsafe fn from_foreign(_: *mut crate::ffi::c_void) -> Self {}
+    unsafe fn from_foreign(_: *mut Self::PointedTo) -> Self {}
 
-    unsafe fn borrow<'a>(_: *mut crate::ffi::c_void) -> Self::Borrowed<'a> {}
-    unsafe fn borrow_mut<'a>(_: *mut crate::ffi::c_void) -> Self::BorrowedMut<'a> {}
+    unsafe fn borrow<'a>(_: *mut Self::PointedTo) -> Self::Borrowed<'a> {}
+    unsafe fn borrow_mut<'a>(_: *mut Self::PointedTo) -> Self::BorrowedMut<'a> {}
 }
 
 /// Runs a cleanup function/closure when dropped.

-- 
2.48.1

[PATCH v15 2/3] rust: xarray: Add an abstraction for XArray

[Tamir Duberstein]

`XArray` is an efficient sparse array of pointers. Add a Rust
abstraction for this type.

This implementation bounds the element type on `ForeignOwnable` and
requires explicit locking for all operations. Future work may leverage
RCU to enable lockless operation.

Inspired-by: Maíra Canal <mcanal@igalia.com>
Inspired-by: Asahi Lina <lina@asahilina.net>
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 rust/bindings/bindings_helper.h |   6 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/xarray.c           |  28 ++++
 rust/kernel/alloc.rs            |   5 +
 rust/kernel/lib.rs              |   1 +
 rust/kernel/xarray.rs           | 279 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 320 insertions(+)

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 55354e4dec14..249070b5abf9 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -34,6 +34,7 @@
 #include <linux/tracepoint.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
+#include <linux/xarray.h>
 #include <trace/events/rust_sample.h>
 
 /* `bindgen` gets confused at certain things. */
@@ -47,3 +48,8 @@ const gfp_t RUST_CONST_HELPER___GFP_ZERO = __GFP_ZERO;
 const gfp_t RUST_CONST_HELPER___GFP_HIGHMEM = ___GFP_HIGHMEM;
 const gfp_t RUST_CONST_HELPER___GFP_NOWARN = ___GFP_NOWARN;
 const blk_features_t RUST_CONST_HELPER_BLK_FEAT_ROTATIONAL = BLK_FEAT_ROTATIONAL;
+
+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;
diff --git a/rust/helpers/helpers.c b/rust/helpers/helpers.c
index 0640b7e115be..6811f71f2cbb 100644
--- a/rust/helpers/helpers.c
+++ b/rust/helpers/helpers.c
@@ -35,3 +35,4 @@
 #include "vmalloc.c"
 #include "wait.c"
 #include "workqueue.c"
+#include "xarray.c"
diff --git a/rust/helpers/xarray.c b/rust/helpers/xarray.c
new file mode 100644
index 000000000000..60b299f11451
--- /dev/null
+++ b/rust/helpers/xarray.c
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/xarray.h>
+
+int rust_helper_xa_err(void *entry)
+{
+	return xa_err(entry);
+}
+
+void rust_helper_xa_init_flags(struct xarray *xa, gfp_t flags)
+{
+	return xa_init_flags(xa, flags);
+}
+
+int rust_helper_xa_trylock(struct xarray *xa)
+{
+	return xa_trylock(xa);
+}
+
+void rust_helper_xa_lock(struct xarray *xa)
+{
+	return xa_lock(xa);
+}
+
+void rust_helper_xa_unlock(struct xarray *xa)
+{
+	return xa_unlock(xa);
+}
diff --git a/rust/kernel/alloc.rs b/rust/kernel/alloc.rs
index fc9c9c41cd79..77840413598d 100644
--- a/rust/kernel/alloc.rs
+++ b/rust/kernel/alloc.rs
@@ -39,6 +39,11 @@
 pub struct Flags(u32);
 
 impl Flags {
+    /// Get a flags value with all bits unset.
+    pub fn empty() -> Self {
+        Self(0)
+    }
+
     /// Get the raw representation of this flag.
     pub(crate) fn as_raw(self) -> u32 {
         self.0
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 496ed32b0911..abe419362c73 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -84,6 +84,7 @@
 pub mod types;
 pub mod uaccess;
 pub mod workqueue;
+pub mod xarray;
 
 #[doc(hidden)]
 pub use bindings;
diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
new file mode 100644
index 000000000000..c62be2a6d85c
--- /dev/null
+++ b/rust/kernel/xarray.rs
@@ -0,0 +1,279 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! XArray abstraction.
+//!
+//! C header: [`include/linux/xarray.h`](srctree/include/linux/xarray.h)
+
+use crate::{
+    alloc, bindings, build_assert,
+    error::{Error, Result},
+    init::PinInit,
+    pin_init,
+    types::{ForeignOwnable, NotThreadSafe, Opaque},
+};
+use core::{iter, marker::PhantomData, mem, pin::Pin, ptr::NonNull};
+use macros::{pin_data, pinned_drop};
+
+/// An array which efficiently maps sparse integer indices to owned objects.
+///
+/// This is similar to a [`crate::alloc::kvec::Vec<Option<T>>`], but more efficient when there are
+/// holes in the index space, and can be efficiently grown.
+///
+/// # Invariants
+///
+/// `self.xa` is always an initialized and valid [`bindings::xarray`] whose entries are either
+/// `XA_ZERO_ENTRY` or came from `T::into_foreign`.
+///
+/// # Examples
+///
+/// ```rust
+/// use kernel::alloc::KBox;
+/// use kernel::xarray::{AllocKind, XArray};
+///
+/// let xa = KBox::pin_init(XArray::new(AllocKind::Alloc1), GFP_KERNEL)?;
+///
+/// let dead = KBox::new(0xdead, GFP_KERNEL)?;
+/// let beef = KBox::new(0xbeef, GFP_KERNEL)?;
+///
+/// let mut guard = xa.lock();
+///
+/// assert_eq!(guard.get(0), None);
+///
+/// assert_eq!(guard.store(0, dead, GFP_KERNEL)?.as_deref(), None);
+/// assert_eq!(guard.get(0).copied(), Some(0xdead));
+///
+/// *guard.get_mut(0).unwrap() = 0xffff;
+/// assert_eq!(guard.get(0).copied(), Some(0xffff));
+///
+/// assert_eq!(guard.store(0, beef, GFP_KERNEL)?.as_deref().copied(), Some(0xffff));
+/// assert_eq!(guard.get(0).copied(), Some(0xbeef));
+///
+/// guard.remove(0);
+/// assert_eq!(guard.get(0), None);
+///
+/// # Ok::<(), Error>(())
+/// ```
+#[pin_data(PinnedDrop)]
+pub struct XArray<T: ForeignOwnable> {
+    #[pin]
+    xa: Opaque<bindings::xarray>,
+    _p: PhantomData<T>,
+}
+
+#[pinned_drop]
+impl<T: ForeignOwnable> PinnedDrop for XArray<T> {
+    fn drop(self: Pin<&mut Self>) {
+        self.iter().for_each(|ptr| {
+            let ptr = ptr.as_ptr();
+            // SAFETY: `ptr` came from `T::into_foreign`.
+            //
+            // INVARIANT: we own the only reference to the array which is being dropped so the
+            // broken invariant is not observable on function exit.
+            drop(unsafe { T::from_foreign(ptr) })
+        });
+
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        unsafe { bindings::xa_destroy(self.xa.get()) };
+    }
+}
+
+/// Flags passed to [`XArray::new`] to configure the array's allocation tracking behavior.
+pub enum AllocKind {
+    /// Consider the first element to be at index 0.
+    Alloc,
+    /// Consider the first element to be at index 1.
+    Alloc1,
+}
+
+impl<T: ForeignOwnable> XArray<T> {
+    /// Creates a new [`XArray`].
+    pub fn new(kind: AllocKind) -> impl PinInit<Self> {
+        let flags = match kind {
+            AllocKind::Alloc => bindings::XA_FLAGS_ALLOC,
+            AllocKind::Alloc1 => bindings::XA_FLAGS_ALLOC1,
+        };
+        pin_init!(Self {
+            // SAFETY: `xa` is valid while the closure is called.
+            xa <- Opaque::ffi_init(|xa| unsafe {
+                bindings::xa_init_flags(xa, flags)
+            }),
+            _p: PhantomData,
+        })
+    }
+
+    fn iter(&self) -> impl Iterator<Item = NonNull<T::PointedTo>> + '_ {
+        let mut index = 0;
+
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        iter::once(unsafe {
+            bindings::xa_find(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
+        })
+        .chain(iter::from_fn(move || {
+            // SAFETY: `self.xa` is always valid by the type invariant.
+            Some(unsafe {
+                bindings::xa_find_after(self.xa.get(), &mut index, usize::MAX, bindings::XA_PRESENT)
+            })
+        }))
+        .map_while(|ptr| NonNull::new(ptr.cast()))
+    }
+
+    /// Attempts to lock the [`XArray`] for exclusive access.
+    pub fn try_lock(&self) -> Option<Guard<'_, T>> {
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        if (unsafe { bindings::xa_trylock(self.xa.get()) } != 0) {
+            Some(Guard {
+                xa: self,
+                _not_send: NotThreadSafe,
+            })
+        } else {
+            None
+        }
+    }
+
+    /// Locks the [`XArray`] for exclusive access.
+    pub fn lock(&self) -> Guard<'_, T> {
+        // SAFETY: `self.xa` is always valid by the type invariant.
+        unsafe { bindings::xa_lock(self.xa.get()) };
+
+        Guard {
+            xa: self,
+            _not_send: NotThreadSafe,
+        }
+    }
+}
+
+/// A lock guard.
+///
+/// The lock is unlocked when the guard goes out of scope.
+#[must_use = "the lock unlocks immediately when the guard is unused"]
+pub struct Guard<'a, T: ForeignOwnable> {
+    xa: &'a XArray<T>,
+    _not_send: NotThreadSafe,
+}
+
+impl<T: ForeignOwnable> Drop for Guard<'_, T> {
+    fn drop(&mut self) {
+        // SAFETY:
+        // - `self.xa.xa` is always valid by the type invariant.
+        // - The caller holds the lock, so it is safe to unlock it.
+        unsafe { bindings::xa_unlock(self.xa.xa.get()) };
+    }
+}
+
+/// The error returned by [`store`](Guard::store).
+///
+/// Contains the underlying error and the value that was not stored.
+pub struct StoreError<T> {
+    /// The error that occurred.
+    pub error: Error,
+    /// The value that was not stored.
+    pub value: T,
+}
+
+impl<T> From<StoreError<T>> for Error {
+    fn from(value: StoreError<T>) -> Self {
+        let StoreError { error, value: _ } = value;
+        error
+    }
+}
+
+impl<'a, T: ForeignOwnable> Guard<'a, T> {
+    fn load<F, U>(&self, index: usize, f: F) -> Option<U>
+    where
+        F: FnOnce(NonNull<T::PointedTo>) -> U,
+    {
+        // SAFETY: `self.xa.xa` is always valid by the type invariant.
+        let ptr = unsafe { bindings::xa_load(self.xa.xa.get(), index) };
+        let ptr = NonNull::new(ptr.cast())?;
+        Some(f(ptr))
+    }
+
+    /// Loads an entry from the array.
+    ///
+    /// Returns the entry 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()) }
+        })
+    }
+
+    /// Loads an entry from the array.
+    ///
+    /// Returns the entry 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()) }
+        })
+    }
+
+    /// Erases an entry from the array.
+    ///
+    /// Returns the entry which was previously at the given index.
+    pub fn remove(&mut self, index: usize) -> Option<T> {
+        // SAFETY: `self.xa.xa` is always valid by the type invariant.
+        //
+        // SAFETY: The caller holds the lock.
+        let ptr = unsafe { bindings::__xa_erase(self.xa.xa.get(), index) }.cast();
+        // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
+        unsafe { T::try_from_foreign(ptr) }
+    }
+
+    /// Stores an entry in the array.
+    ///
+    /// May drop the lock if needed to allocate memory, and then reacquire it afterwards.
+    ///
+    /// On success, returns the entry which was previously at the given index.
+    ///
+    /// On failure, returns the entry which was attempted to be stored.
+    pub fn store(
+        &mut self,
+        index: usize,
+        value: T,
+        gfp: alloc::Flags,
+    ) -> Result<Option<T>, StoreError<T>> {
+        build_assert!(
+            mem::align_of::<T::PointedTo>() >= 4,
+            "pointers stored in XArray must be 4-byte aligned"
+        );
+        let new = value.into_foreign();
+
+        let old = {
+            let new = new.cast();
+            // SAFETY: `self.xa.xa` is always valid by the type invariant.
+            //
+            // SAFETY: The caller holds the lock.
+            //
+            // INVARIANT: `new` came from `T::into_foreign`.
+            unsafe { bindings::__xa_store(self.xa.xa.get(), index, new, gfp.as_raw()) }
+        };
+
+        // SAFETY: `__xa_store` returns the old entry at this index on success or `xa_err` if an
+        // error happened.
+        let errno = unsafe { bindings::xa_err(old) };
+        if errno != 0 {
+            // SAFETY: `new` came from `T::into_foreign` and `__xa_store` does not take
+            // ownership of the value on error.
+            let value = unsafe { T::from_foreign(new) };
+            Err(StoreError {
+                value,
+                error: Error::from_errno(errno),
+            })
+        } else {
+            let old = old.cast();
+            // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
+            //
+            // NB: `XA_ZERO_ENTRY` is never returned by functions belonging to the Normal XArray
+            // API; such entries present as `NULL`.
+            Ok(unsafe { T::try_from_foreign(old) })
+        }
+    }
+}
+
+// SAFETY: `XArray<T>` has no shared mutable state so it is `Send` iff `T` is `Send`.
+unsafe impl<T: ForeignOwnable + Send> Send for XArray<T> {}
+
+// SAFETY: `XArray<T>` serialises the interior mutability it provides so it is `Sync` iff `T` is
+// `Send`.
+unsafe impl<T: ForeignOwnable + Send> Sync for XArray<T> {}

-- 
2.48.1

[PATCH v15 3/3] MAINTAINERS: add entry for Rust XArray API

[Tamir Duberstein]

Add an entry for the Rust xarray abstractions.

Signed-off-by: Tamir Duberstein <tamird@gmail.com>
---
 MAINTAINERS | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 896a307fa065..88282b6e689b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25749,6 +25749,17 @@ F:	lib/test_xarray.c
 F:	lib/xarray.c
 F:	tools/testing/radix-tree
 
+XARRAY API [RUST]
+M:	Tamir Duberstein <tamird@gmail.com>
+M:	Andreas Hindborg <a.hindborg@kernel.org>
+L:	rust-for-linux@vger.kernel.org
+S:	Supported
+W:	https://rust-for-linux.com
+B:	https://github.com/Rust-for-Linux/linux/issues
+C:	https://rust-for-linux.zulipchat.com
+T:	git https://github.com/Rust-for-Linux/linux.git rust-next
+F:	rust/kernel/xarray.rs
+
 XBOX DVD IR REMOTE
 M:	Benjamin Valentin <benpicco@googlemail.com>
 S:	Maintained

-- 
2.48.1

Re: [PATCH v15 1/3] rust: types: add `ForeignOwnable::PointedTo`

[Danilo Krummrich]

On Thu, Feb 06, 2025 at 11:24:43AM -0500, Tamir Duberstein wrote:
> Allow implementors to specify the foreign pointer type; this exposes
> information about the pointed-to type such as its alignment.
> 
> This requires the trait to be `unsafe` since it is now possible for
> implementors to break soundness by returning a misaligned pointer.
> 
> Encoding the pointer type in the trait (and avoiding pointer casts)
> allows the compiler to check that implementors return the correct
> pointer type. This is preferable to directly encoding the alignment in
> the trait using a constant as the compiler would be unable to check it.
> 
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
>  rust/kernel/miscdevice.rs |  7 ++++++-
>  rust/kernel/pci.rs        |  5 ++++-
>  rust/kernel/platform.rs   |  5 ++++-
>  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
>  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
>  6 files changed, 77 insertions(+), 45 deletions(-)
> 
> diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> index e14433b2ab9d..f1a081dd64c7 100644
> --- a/rust/kernel/miscdevice.rs
> +++ b/rust/kernel/miscdevice.rs
> @@ -225,13 +225,15 @@ impl<T: MiscDevice> VtableHelper<T> {
>          Ok(ptr) => ptr,
>          Err(err) => return err.to_errno(),
>      };
> +    let ptr = ptr.into_foreign();
> +    let ptr = ptr.cast();
>  
>      // This overwrites the private data with the value specified by the user, changing the type of
>      // this file's private data. All future accesses to the private data is performed by other
>      // fops_* methods in this file, which all correctly cast the private data to the new type.
>      //
>      // SAFETY: The open call of a file can access the private data.
> -    unsafe { (*raw_file).private_data = ptr.into_foreign() };
> +    unsafe { (*raw_file).private_data = ptr };

Why not just ptr.into_foreign().cast()?

>  
>      0
>  }
> @@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> {
>  ) -> c_int {
>      // SAFETY: The release call of a file owns the private data.
>      let private = unsafe { (*file).private_data };
> +    let private = private.cast();
>      // SAFETY: The release call of a file owns the private data.
>      let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
>  
> @@ -267,6 +270,7 @@ impl<T: MiscDevice> VtableHelper<T> {
>  ) -> c_long {
>      // SAFETY: The ioctl call of a file can access the private data.
>      let private = unsafe { (*file).private_data };
> +    let private = private.cast();
>      // SAFETY: Ioctl calls can borrow the private data of the file.
>      let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
>  
> @@ -316,6 +320,7 @@ impl<T: MiscDevice> VtableHelper<T> {
>  ) {
>      // SAFETY: The release call of a file owns the private data.
>      let private = unsafe { (*file).private_data };
> +    let private = private.cast();
>      // SAFETY: Ioctl calls can borrow the private data of the file.
>      let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
>      // SAFETY:
> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 4c98b5b9aa1e..eb25fabbff9c 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -72,10 +72,12 @@ extern "C" fn probe_callback(
>  
>          match T::probe(&mut pdev, info) {
>              Ok(data) => {
> +                let data = data.into_foreign();
> +                let data = data.cast();
>                  // Let the `struct pci_dev` own a reference of the driver's private data.
>                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
>                  // `struct pci_dev`.
> -                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> +                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data) };

This change isn't necessary for this patch, is it? I think it makes sense to
replace `as _` with cast(), but this should be a separate patch then.

>              }
>              Err(err) => return Error::to_errno(err),
>          }
> @@ -87,6 +89,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
>          // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
>          // `struct pci_dev`.
>          let ptr = unsafe { bindings::pci_get_drvdata(pdev) };
> +        let ptr = ptr.cast();
>  
>          // SAFETY: `remove_callback` is only ever called after a successful call to
>          // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
> diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> index 50e6b0421813..53764cb7f804 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -63,10 +63,12 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
>          let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
>          match T::probe(&mut pdev, info) {
>              Ok(data) => {
> +                let data = data.into_foreign();
> +                let data = data.cast();
>                  // Let the `struct platform_device` own a reference of the driver's private data.
>                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
>                  // `struct platform_device`.
> -                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> +                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data) };

Same here.

Re: [PATCH v15 2/3] rust: xarray: Add an abstraction for XArray

[Boqun Feng]

Hi Tamir,

This looks good to me overall, a few comments below:

On Thu, Feb 06, 2025 at 11:24:44AM -0500, Tamir Duberstein wrote:
[...]
> +impl<'a, T: ForeignOwnable> Guard<'a, T> {
[...]
> +    /// Loads an entry from the array.
> +    ///
> +    /// Returns the entry 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()) }
> +        })
> +    }
> +
> +    /// Loads an entry from the array.

Nit: firstly, this function has the same description of `get()`, also
I would prefer something like "Returns a [`T::Borrowed`] of the object
at `index`" rather then "Loads an entry from the array", thoughts?

> +    ///
> +    /// Returns the entry 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()) }
> +        })
> +    }
> +
> +    /// Erases an entry from the array.

Nit: s/Erases/Removes?

> +    ///
> +    /// Returns the entry which was previously at the given index.
> +    pub fn remove(&mut self, index: usize) -> Option<T> {
> +        // SAFETY: `self.xa.xa` is always valid by the type invariant.
> +        //
> +        // SAFETY: The caller holds the lock.
> +        let ptr = unsafe { bindings::__xa_erase(self.xa.xa.get(), index) }.cast();
> +        // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.

SAFETY comment here needs to mention why there is no alive `T::Borrowed`
or `T::BorrowedMut` out there per the safety requirement.

Regards,
Boqun

> +        unsafe { T::try_from_foreign(ptr) }
> +    }
> +
[...]

Re: [PATCH v15 1/3] rust: types: add `ForeignOwnable::PointedTo`

[Tamir Duberstein]

On Thu, Feb 6, 2025 at 11:39 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Thu, Feb 06, 2025 at 11:24:43AM -0500, Tamir Duberstein wrote:
> > Allow implementors to specify the foreign pointer type; this exposes
> > information about the pointed-to type such as its alignment.
> >
> > This requires the trait to be `unsafe` since it is now possible for
> > implementors to break soundness by returning a misaligned pointer.
> >
> > Encoding the pointer type in the trait (and avoiding pointer casts)
> > allows the compiler to check that implementors return the correct
> > pointer type. This is preferable to directly encoding the alignment in
> > the trait using a constant as the compiler would be unable to check it.
> >
> > Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> > Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> > ---
> >  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
> >  rust/kernel/miscdevice.rs |  7 ++++++-
> >  rust/kernel/pci.rs        |  5 ++++-
> >  rust/kernel/platform.rs   |  5 ++++-
> >  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
> >  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
> >  6 files changed, 77 insertions(+), 45 deletions(-)
> >
> > diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
> > index e14433b2ab9d..f1a081dd64c7 100644
> > --- a/rust/kernel/miscdevice.rs
> > +++ b/rust/kernel/miscdevice.rs
> > @@ -225,13 +225,15 @@ impl<T: MiscDevice> VtableHelper<T> {
> >          Ok(ptr) => ptr,
> >          Err(err) => return err.to_errno(),
> >      };
> > +    let ptr = ptr.into_foreign();
> > +    let ptr = ptr.cast();
> >
> >      // This overwrites the private data with the value specified by the user, changing the type of
> >      // this file's private data. All future accesses to the private data is performed by other
> >      // fops_* methods in this file, which all correctly cast the private data to the new type.
> >      //
> >      // SAFETY: The open call of a file can access the private data.
> > -    unsafe { (*raw_file).private_data = ptr.into_foreign() };
> > +    unsafe { (*raw_file).private_data = ptr };
>
> Why not just ptr.into_foreign().cast()?

That would work too. I was trying to move stuff out of the unsafe block.

> >
> >      0
> >  }
> > @@ -246,6 +248,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> >  ) -> c_int {
> >      // SAFETY: The release call of a file owns the private data.
> >      let private = unsafe { (*file).private_data };
> > +    let private = private.cast();
> >      // SAFETY: The release call of a file owns the private data.
> >      let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
> >
> > @@ -267,6 +270,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> >  ) -> c_long {
> >      // SAFETY: The ioctl call of a file can access the private data.
> >      let private = unsafe { (*file).private_data };
> > +    let private = private.cast();
> >      // SAFETY: Ioctl calls can borrow the private data of the file.
> >      let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> >
> > @@ -316,6 +320,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> >  ) {
> >      // SAFETY: The release call of a file owns the private data.
> >      let private = unsafe { (*file).private_data };
> > +    let private = private.cast();
> >      // SAFETY: Ioctl calls can borrow the private data of the file.
> >      let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
> >      // SAFETY:
> > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > index 4c98b5b9aa1e..eb25fabbff9c 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -72,10 +72,12 @@ extern "C" fn probe_callback(
> >
> >          match T::probe(&mut pdev, info) {
> >              Ok(data) => {
> > +                let data = data.into_foreign();
> > +                let data = data.cast();
> >                  // Let the `struct pci_dev` own a reference of the driver's private data.
> >                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> >                  // `struct pci_dev`.
> > -                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> > +                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data) };
>
> This change isn't necessary for this patch, is it? I think it makes sense to
> replace `as _` with cast(), but this should be a separate patch then.

Sure, I can make this a separate (prequel) patch.

> >              }
> >              Err(err) => return Error::to_errno(err),
> >          }
> > @@ -87,6 +89,7 @@ extern "C" fn remove_callback(pdev: *mut bindings::pci_dev) {
> >          // SAFETY: The PCI bus only ever calls the remove callback with a valid pointer to a
> >          // `struct pci_dev`.
> >          let ptr = unsafe { bindings::pci_get_drvdata(pdev) };
> > +        let ptr = ptr.cast();
> >
> >          // SAFETY: `remove_callback` is only ever called after a successful call to
> >          // `probe_callback`, hence it's guaranteed that `ptr` points to a valid and initialized
> > diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
> > index 50e6b0421813..53764cb7f804 100644
> > --- a/rust/kernel/platform.rs
> > +++ b/rust/kernel/platform.rs
> > @@ -63,10 +63,12 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
> >          let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
> >          match T::probe(&mut pdev, info) {
> >              Ok(data) => {
> > +                let data = data.into_foreign();
> > +                let data = data.cast();
> >                  // Let the `struct platform_device` own a reference of the driver's private data.
> >                  // SAFETY: By the type invariant `pdev.as_raw` returns a valid pointer to a
> >                  // `struct platform_device`.
> > -                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> > +                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data) };
>
> Same here.

Yep. Will do.

Re: [PATCH v15 2/3] rust: xarray: Add an abstraction for XArray

[Tamir Duberstein]

Hi Boqun,

On Thu, Feb 6, 2025 at 12:18 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Hi Tamir,
>
> This looks good to me overall, a few comments below:
>
> On Thu, Feb 06, 2025 at 11:24:44AM -0500, Tamir Duberstein wrote:
> [...]
> > +impl<'a, T: ForeignOwnable> Guard<'a, T> {
> [...]
> > +    /// Loads an entry from the array.
> > +    ///
> > +    /// Returns the entry 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()) }
> > +        })
> > +    }
> > +
> > +    /// Loads an entry from the array.
>
> Nit: firstly, this function has the same description of `get()`, also
> I would prefer something like "Returns a [`T::Borrowed`] of the object
> at `index`" rather then "Loads an entry from the array", thoughts?

I was trying to avoid repeating the signature in the comment. In other
words I was trying to write a comment that wouldn't have to change if
the signature (but not the semantics) of the function changed. Since
the difference between `get` and `get_mut` is completely described in
the type system, the two functions got the same comment. Shall I
change it?

> > +    ///
> > +    /// Returns the entry 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()) }
> > +        })
> > +    }
> > +
> > +    /// Erases an entry from the array.
>
> Nit: s/Erases/Removes?

Will change. I used "erase" because that's the verb used in the C
function name but named it "remove" because that's the verb used in
the Rust standard library. The result is neither here nor there :)

>
> > +    ///
> > +    /// Returns the entry which was previously at the given index.
> > +    pub fn remove(&mut self, index: usize) -> Option<T> {
> > +        // SAFETY: `self.xa.xa` is always valid by the type invariant.
> > +        //
> > +        // SAFETY: The caller holds the lock.
> > +        let ptr = unsafe { bindings::__xa_erase(self.xa.xa.get(), index) }.cast();
> > +        // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
>
> SAFETY comment here needs to mention why there is no alive `T::Borrowed`
> or `T::BorrowedMut` out there per the safety requirement.

Will do.

> Regards,
> Boqun
>
> > +        unsafe { T::try_from_foreign(ptr) }
> > +    }
> > +
> [...]

Thanks for the review!

Re: [PATCH v15 1/3] rust: types: add `ForeignOwnable::PointedTo`

[Andreas Hindborg]

"Tamir Duberstein" <tamird@gmail.com> writes:

> Allow implementors to specify the foreign pointer type; this exposes
> information about the pointed-to type such as its alignment.
>
> This requires the trait to be `unsafe` since it is now possible for
> implementors to break soundness by returning a misaligned pointer.
>
> Encoding the pointer type in the trait (and avoiding pointer casts)
> allows the compiler to check that implementors return the correct
> pointer type. This is preferable to directly encoding the alignment in
> the trait using a constant as the compiler would be unable to check it.
>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---

Pleas pick up tags from Alice and Fiona from the configfs series where I
included your patch:

https://lore.kernel.org/rust-for-linux/20250131-configfs-v1-1-87947611401c@kernel.org/


Best regards,
Andreas Hindborg

Re: [PATCH v15 3/3] MAINTAINERS: add entry for Rust XArray API

[Andreas Hindborg]

"Tamir Duberstein" <tamird@gmail.com> writes:

> Add an entry for the Rust xarray abstractions.
>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>

Acked-by: Andreas Hindborg <a.hindborg@kernel.org>


Best regards,
Andreas Hindborg

Re: [PATCH v15 2/3] rust: xarray: Add an abstraction for XArray

[Boqun Feng]

On Thu, Feb 06, 2025 at 01:21:35PM -0500, Tamir Duberstein wrote:
> Hi Boqun,
> 
> On Thu, Feb 6, 2025 at 12:18 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> >
> > Hi Tamir,
> >
> > This looks good to me overall, a few comments below:
> >
> > On Thu, Feb 06, 2025 at 11:24:44AM -0500, Tamir Duberstein wrote:
> > [...]
> > > +impl<'a, T: ForeignOwnable> Guard<'a, T> {
> > [...]
> > > +    /// Loads an entry from the array.
> > > +    ///
> > > +    /// Returns the entry 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()) }
> > > +        })
> > > +    }
> > > +
> > > +    /// Loads an entry from the array.
> >
> > Nit: firstly, this function has the same description of `get()`, also
> > I would prefer something like "Returns a [`T::Borrowed`] of the object
> > at `index`" rather then "Loads an entry from the array", thoughts?
> 
> I was trying to avoid repeating the signature in the comment. In other
> words I was trying to write a comment that wouldn't have to change if
> the signature (but not the semantics) of the function changed. Since

Understood. However, I think doc comments and function signatures (and
name) can have the overlapped information, because they are for
different users. Surely a developer who already knows what XArray is
will make a good guess on what `get()` and `get_mut()` do, but it won't
hurt to have the doc comments double-confirming the guess. Besides there
could also be someone who is not that familiar with XArray and would
like to seek the information from the doc comments at first, then having
a more precise description would be helpful.

> the difference between `get` and `get_mut` is completely described in
> the type system, the two functions got the same comment. Shall I
> change it?
> 

Your call ;-) It's a nitpicking after all, and you're the maintainer.
However, I do want to make the point that being a bit more comprehensive
won't hurt when providing an API.

Regards,
Boqun

> > > +    ///
> > > +    /// Returns the entry 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()) }
> > > +        })
> > > +    }
> > > +
> > > +    /// Erases an entry from the array.
> >
> > Nit: s/Erases/Removes?
> 
> Will change. I used "erase" because that's the verb used in the C
> function name but named it "remove" because that's the verb used in
> the Rust standard library. The result is neither here nor there :)
> 
> >
> > > +    ///
> > > +    /// Returns the entry which was previously at the given index.
> > > +    pub fn remove(&mut self, index: usize) -> Option<T> {
> > > +        // SAFETY: `self.xa.xa` is always valid by the type invariant.
> > > +        //
> > > +        // SAFETY: The caller holds the lock.
> > > +        let ptr = unsafe { bindings::__xa_erase(self.xa.xa.get(), index) }.cast();
> > > +        // SAFETY: `ptr` is either NULL or came from `T::into_foreign`.
> >
> > SAFETY comment here needs to mention why there is no alive `T::Borrowed`
> > or `T::BorrowedMut` out there per the safety requirement.
> 
> Will do.
> 
> > Regards,
> > Boqun
> >
> > > +        unsafe { T::try_from_foreign(ptr) }
> > > +    }
> > > +
> > [...]
> 
> Thanks for the review!

Re: [PATCH v15 2/3] rust: xarray: Add an abstraction for XArray

[Tamir Duberstein]

On Thu, Feb 6, 2025 at 3:58 PM Boqun Feng <boqun.feng@gmail.com> wrote:
>
> Your call ;-) It's a nitpicking after all, and you're the maintainer.
> However, I do want to make the point that being a bit more comprehensive
> won't hurt when providing an API.
>
> Regards,
> Boqun

Point taken, and I appreciate your feedback! I went with something a
bit more informative without being repetitive, inspired by std again.
Will send v16 tomorrow.

Cheers.
Tamir