[PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Andreas Hindborg]

The current implementation of `ForeignOwnable` is leaking the type of the
opaque pointer to consumers of the API. This allows consumers of the opaque
pointer to rely on the information that can be extracted from the pointer
type.

To prevent this, change the API to the version suggested by Maira
Canal (link below): Remove `ForeignOwnable::PointedTo` in favor of a
constant, which specifies the alignment of the pointers returned by
`into_foreign`.

With this change, `ArcInner` no longer needs `pub` visibility, so change it
to private.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Suggested-by: Maíra Canal <mcanal@igalia.com>
Link: https://lore.kernel.org/r/20240309235927.168915-3-mcanal@igalia.com
Acked-by: Danilo Krummrich <dakr@kernel.org>
Reviewed-by: Benno Lossin <lossin@kernel.org>
Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
---
Changes in v2:
- Replace qualified path with `use` for `crate::ffi::c_void`.
- Fix a typo and rephrase docs for `ForeignOwnable`.
- Reorganize docs for `ForeignOwnable::into_foreign`.
- Link to v1: https://lore.kernel.org/r/20250605-pointed-to-v1-1-ee1e262912cc@kernel.org
---
 rust/kernel/alloc/kbox.rs | 41 +++++++++++++++++++++++------------------
 rust/kernel/miscdevice.rs | 10 +++++-----
 rust/kernel/pci.rs        |  2 +-
 rust/kernel/platform.rs   |  2 +-
 rust/kernel/sync/arc.rs   | 23 ++++++++++++-----------
 rust/kernel/types.rs      | 45 ++++++++++++++++++++++-----------------------
 rust/kernel/xarray.rs     |  8 ++++----
 7 files changed, 68 insertions(+), 63 deletions(-)

diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
index c386ff771d50..bffe72f44cb3 100644
--- a/rust/kernel/alloc/kbox.rs
+++ b/rust/kernel/alloc/kbox.rs
@@ -15,6 +15,7 @@
 use core::ptr::NonNull;
 use core::result::Result;
 
+use crate::ffi::c_void;
 use crate::init::InPlaceInit;
 use crate::types::ForeignOwnable;
 use pin_init::{InPlaceWrite, Init, PinInit, ZeroableOption};
@@ -398,70 +399,74 @@ fn try_init<E>(init: impl Init<T, E>, flags: Flags) -> Result<Self, E>
     }
 }
 
-// SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
+// SAFETY: The pointer returned by `into_foreign` comes from a well aligned
+// pointer to `T`.
 unsafe impl<T: 'static, A> ForeignOwnable for Box<T, A>
 where
     A: Allocator,
 {
-    type PointedTo = T;
+    const FOREIGN_ALIGN: usize = core::mem::align_of::<T>();
     type Borrowed<'a> = &'a T;
     type BorrowedMut<'a> = &'a mut T;
 
-    fn into_foreign(self) -> *mut Self::PointedTo {
-        Box::into_raw(self)
+    fn into_foreign(self) -> *mut c_void {
+        Box::into_raw(self).cast()
     }
 
-    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
+    unsafe fn from_foreign(ptr: *mut c_void) -> 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) }
+        unsafe { Box::from_raw(ptr.cast()) }
     }
 
-    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> &'a T {
+    unsafe fn borrow<'a>(ptr: *mut c_void) -> &'a T {
         // SAFETY: The safety requirements of this method ensure that the object remains alive and
         // immutable for the duration of 'a.
-        unsafe { &*ptr }
+        unsafe { &*ptr.cast() }
     }
 
-    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> &'a mut T {
+    unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> &'a mut T {
+        let ptr = ptr.cast();
         // 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 }
     }
 }
 
-// SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
+// SAFETY: The pointer returned by `into_foreign` comes from a well aligned
+// pointer to `T`.
 unsafe impl<T: 'static, A> ForeignOwnable for Pin<Box<T, A>>
 where
     A: Allocator,
 {
-    type PointedTo = T;
+    const FOREIGN_ALIGN: usize = core::mem::align_of::<T>();
     type Borrowed<'a> = Pin<&'a T>;
     type BorrowedMut<'a> = Pin<&'a mut T>;
 
-    fn into_foreign(self) -> *mut Self::PointedTo {
+    fn into_foreign(self) -> *mut c_void {
         // SAFETY: We are still treating the box as pinned.
-        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) })
+        Box::into_raw(unsafe { Pin::into_inner_unchecked(self) }).cast()
     }
 
-    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
+    unsafe fn from_foreign(ptr: *mut c_void) -> 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)) }
+        unsafe { Pin::new_unchecked(Box::from_raw(ptr.cast())) }
     }
 
-    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Pin<&'a T> {
+    unsafe fn borrow<'a>(ptr: *mut c_void) -> 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 };
+        let r = unsafe { &*ptr.cast() };
 
         // SAFETY: This pointer originates from a `Pin<Box<T>>`.
         unsafe { Pin::new_unchecked(r) }
     }
 
-    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> Pin<&'a mut T> {
+    unsafe fn borrow_mut<'a>(ptr: *mut c_void) -> Pin<&'a mut T> {
+        let ptr = ptr.cast();
         // 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 f33c13c3ff97..9b46c3f7ac65 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -217,7 +217,7 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         // type.
         //
         // SAFETY: The open call of a file can access the private data.
-        unsafe { (*raw_file).private_data = ptr.into_foreign().cast() };
+        unsafe { (*raw_file).private_data = ptr.into_foreign() };
 
         0
     }
@@ -228,7 +228,7 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
     /// must be associated with a `MiscDeviceRegistration<T>`.
     unsafe extern "C" fn release(_inode: *mut bindings::inode, file: *mut bindings::file) -> c_int {
         // SAFETY: The release call of a file owns the private data.
-        let private = unsafe { (*file).private_data }.cast();
+        let private = unsafe { (*file).private_data };
         // SAFETY: The release call of a file owns the private data.
         let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
 
@@ -272,7 +272,7 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
     /// `file` must be a valid file that is associated with a `MiscDeviceRegistration<T>`.
     unsafe extern "C" fn ioctl(file: *mut bindings::file, cmd: c_uint, arg: c_ulong) -> c_long {
         // SAFETY: The ioctl call of a file can access the private data.
-        let private = unsafe { (*file).private_data }.cast();
+        let private = unsafe { (*file).private_data };
         // SAFETY: Ioctl calls can borrow the private data of the file.
         let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
 
@@ -297,7 +297,7 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
         arg: c_ulong,
     ) -> c_long {
         // SAFETY: The compat ioctl call of a file can access the private data.
-        let private = unsafe { (*file).private_data }.cast();
+        let private = unsafe { (*file).private_data };
         // SAFETY: Ioctl calls can borrow the private data of the file.
         let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
 
@@ -318,7 +318,7 @@ impl<T: MiscDevice> MiscdeviceVTable<T> {
     /// - `seq_file` must be a valid `struct seq_file` that we can write to.
     unsafe extern "C" fn show_fdinfo(seq_file: *mut bindings::seq_file, file: *mut bindings::file) {
         // SAFETY: The release call of a file owns the private data.
-        let private = unsafe { (*file).private_data }.cast();
+        let private = unsafe { (*file).private_data };
         // 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 8435f8132e38..0b4b52804250 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -89,7 +89,7 @@ extern "C" fn probe_callback(
     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) }.cast();
+        let ptr = unsafe { bindings::pci_get_drvdata(pdev) };
 
         // 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 5b21fa517e55..4e37c5ab014d 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -79,7 +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) }.cast();
+        let ptr = unsafe { bindings::platform_get_drvdata(pdev) };
 
         // 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 c7af0aa48a0a..6603079b05af 100644
--- a/rust/kernel/sync/arc.rs
+++ b/rust/kernel/sync/arc.rs
@@ -140,10 +140,9 @@ pub struct Arc<T: ?Sized> {
     _p: PhantomData<ArcInner<T>>,
 }
 
-#[doc(hidden)]
 #[pin_data]
 #[repr(C)]
-pub struct ArcInner<T: ?Sized> {
+struct ArcInner<T: ?Sized> {
     refcount: Opaque<bindings::refcount_t>,
     data: T,
 }
@@ -372,20 +371,22 @@ pub fn into_unique_or_drop(self) -> Option<Pin<UniqueArc<T>>> {
     }
 }
 
-// SAFETY: The `into_foreign` function returns a pointer that is well-aligned.
+// SAFETY: The pointer returned by `into_foreign` comes from a well aligned
+// pointer to `ArcInner<T>`.
 unsafe impl<T: 'static> ForeignOwnable for Arc<T> {
-    type PointedTo = ArcInner<T>;
+    const FOREIGN_ALIGN: usize = core::mem::align_of::<ArcInner<T>>();
+
     type Borrowed<'a> = ArcBorrow<'a, T>;
     type BorrowedMut<'a> = Self::Borrowed<'a>;
 
-    fn into_foreign(self) -> *mut Self::PointedTo {
-        ManuallyDrop::new(self).ptr.as_ptr()
+    fn into_foreign(self) -> *mut crate::ffi::c_void {
+        ManuallyDrop::new(self).ptr.as_ptr().cast()
     }
 
-    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
+    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> 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) };
+        let inner = unsafe { NonNull::new_unchecked(ptr.cast::<ArcInner<T>>()) };
 
         // 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
@@ -393,17 +394,17 @@ unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self {
         unsafe { Self::from_inner(inner) }
     }
 
-    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> ArcBorrow<'a, T> {
+    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> 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) };
+        let inner = unsafe { NonNull::new_unchecked(ptr.cast::<ArcInner<T>>()) };
 
         // 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 Self::PointedTo) -> ArcBorrow<'a, T> {
+    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> 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 22985b6f6982..0ccef6b5a20a 100644
--- a/rust/kernel/types.rs
+++ b/rust/kernel/types.rs
@@ -21,15 +21,11 @@
 ///
 /// # 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
+/// Implementers must ensure that [`Self::into_foreign`] returns pointers aligned to
+/// [`Self::FOREIGN_ALIGN`].
 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;
+    /// The alignment of pointers returned by `into_foreign`.
+    const FOREIGN_ALIGN: usize;
 
     /// Type used to immutably borrow a value that is currently foreign-owned.
     type Borrowed<'a>;
@@ -39,18 +35,20 @@ pub unsafe trait ForeignOwnable: Sized {
 
     /// Converts a Rust-owned object to a foreign-owned one.
     ///
+    /// The foreign representation is a pointer to void. Aside from the guarantees listed below,
+    /// there are no other 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.
+    /// - Minimum alignment of returned pointer is [`Self::FOREIGN_ALIGN`].
     ///
     /// [`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 Self::PointedTo;
+    fn into_foreign(self) -> *mut crate::ffi::c_void;
 
     /// Converts a foreign-owned object back to a Rust-owned one.
     ///
@@ -60,7 +58,7 @@ pub unsafe trait ForeignOwnable: Sized {
     /// must not be passed to `from_foreign` more than once.
     ///
     /// [`into_foreign`]: Self::into_foreign
-    unsafe fn from_foreign(ptr: *mut Self::PointedTo) -> Self;
+    unsafe fn from_foreign(ptr: *mut crate::ffi::c_void) -> Self;
 
     /// Tries to convert a foreign-owned object back to a Rust-owned one.
     ///
@@ -72,7 +70,7 @@ pub unsafe 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 Self::PointedTo) -> Option<Self> {
+    unsafe fn try_from_foreign(ptr: *mut crate::ffi::c_void) -> Option<Self> {
         if ptr.is_null() {
             None
         } else {
@@ -95,7 +93,7 @@ unsafe fn try_from_foreign(ptr: *mut Self::PointedTo) -> Option<Self> {
     ///
     /// [`into_foreign`]: Self::into_foreign
     /// [`from_foreign`]: Self::from_foreign
-    unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Self::Borrowed<'a>;
+    unsafe fn borrow<'a>(ptr: *mut crate::ffi::c_void) -> Self::Borrowed<'a>;
 
     /// Borrows a foreign-owned object mutably.
     ///
@@ -123,23 +121,24 @@ unsafe fn try_from_foreign(ptr: *mut Self::PointedTo) -> Option<Self> {
     /// [`from_foreign`]: Self::from_foreign
     /// [`borrow`]: Self::borrow
     /// [`Arc`]: crate::sync::Arc
-    unsafe fn borrow_mut<'a>(ptr: *mut Self::PointedTo) -> Self::BorrowedMut<'a>;
+    unsafe fn borrow_mut<'a>(ptr: *mut crate::ffi::c_void) -> Self::BorrowedMut<'a>;
 }
 
-// SAFETY: The `into_foreign` function returns a pointer that is dangling, but well-aligned.
+// SAFETY: The pointer returned by `into_foreign` comes from a well aligned
+// pointer to `()`.
 unsafe impl ForeignOwnable for () {
-    type PointedTo = ();
+    const FOREIGN_ALIGN: usize = core::mem::align_of::<()>();
     type Borrowed<'a> = ();
     type BorrowedMut<'a> = ();
 
-    fn into_foreign(self) -> *mut Self::PointedTo {
+    fn into_foreign(self) -> *mut crate::ffi::c_void {
         core::ptr::NonNull::dangling().as_ptr()
     }
 
-    unsafe fn from_foreign(_: *mut Self::PointedTo) -> Self {}
+    unsafe fn from_foreign(_: *mut crate::ffi::c_void) -> Self {}
 
-    unsafe fn borrow<'a>(_: *mut Self::PointedTo) -> Self::Borrowed<'a> {}
-    unsafe fn borrow_mut<'a>(_: *mut Self::PointedTo) -> Self::BorrowedMut<'a> {}
+    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> {}
 }
 
 /// Runs a cleanup function/closure when dropped.
diff --git a/rust/kernel/xarray.rs b/rust/kernel/xarray.rs
index 75719e7bb491..35f4357fc03a 100644
--- a/rust/kernel/xarray.rs
+++ b/rust/kernel/xarray.rs
@@ -9,7 +9,7 @@
     error::{Error, Result},
     types::{ForeignOwnable, NotThreadSafe, Opaque},
 };
-use core::{iter, marker::PhantomData, mem, pin::Pin, ptr::NonNull};
+use core::{iter, marker::PhantomData, pin::Pin, ptr::NonNull};
 use pin_init::{pin_data, pin_init, pinned_drop, PinInit};
 
 /// An array which efficiently maps sparse integer indices to owned objects.
@@ -101,7 +101,7 @@ pub fn new(kind: AllocKind) -> impl PinInit<Self> {
         })
     }
 
-    fn iter(&self) -> impl Iterator<Item = NonNull<T::PointedTo>> + '_ {
+    fn iter(&self) -> impl Iterator<Item = NonNull<crate::ffi::c_void>> + '_ {
         let mut index = 0;
 
         // SAFETY: `self.xa` is always valid by the type invariant.
@@ -179,7 +179,7 @@ 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<T::PointedTo>) -> U,
+        F: FnOnce(NonNull<crate::ffi::c_void>) -> U,
     {
         // SAFETY: `self.xa.xa` is always valid by the type invariant.
         let ptr = unsafe { bindings::xa_load(self.xa.xa.get(), index) };
@@ -230,7 +230,7 @@ pub fn store(
         gfp: alloc::Flags,
     ) -> Result<Option<T>, StoreError<T>> {
         build_assert!(
-            mem::align_of::<T::PointedTo>() >= 4,
+            T::FOREIGN_ALIGN >= 4,
             "pointers stored in XArray must be 4-byte aligned"
         );
         let new = value.into_foreign();

---
base-commit: ec7714e4947909190ffb3041a03311a975350fe0
change-id: 20250605-pointed-to-6170ae01520f

Best regards,
-- 
Andreas Hindborg <a.hindborg@kernel.org>

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Miguel Ojeda]

On Tue, Jun 10, 2025 at 1:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> - Replace qualified path with `use` for `crate::ffi::c_void`.

Hmm... I think parts of the patch still use the qualified path?

Cheers,
Miguel

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Andreas Hindborg]

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

> On Tue, Jun 10, 2025 at 1:32 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> - Replace qualified path with `use` for `crate::ffi::c_void`.
>
> Hmm... I think parts of the patch still use the qualified path?

Yea, sorry. I just changed the kbox file where Benno pointed this out. I
can change the rest of the places as well.


Best regards,
Andreas Hindborg

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Miguel Ojeda]

On Tue, Jun 10, 2025 at 1:58 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> Yea, sorry. I just changed the kbox file where Benno pointed this out. I
> can change the rest of the places as well.

No worries!

If they are new cases, then I would use the unqualified ones (which
are also in the prelude now), since we are trying to more towards that
(commit 3d5bef5d47c3 ("rust: add C FFI types to the prelude")).

That way we have fewer to clean later on.

Thanks!

Cheers,
Miguel

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Benno Lossin]

On Tue Jun 10, 2025 at 1:30 PM CEST, Andreas Hindborg wrote:
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 22985b6f6982..0ccef6b5a20a 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
> @@ -21,15 +21,11 @@
>  ///
>  /// # 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
> +/// Implementers must ensure that [`Self::into_foreign`] returns pointers aligned to
> +/// [`Self::FOREIGN_ALIGN`].
>  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;
> +    /// The alignment of pointers returned by `into_foreign`.
> +    const FOREIGN_ALIGN: usize;
>  
>      /// Type used to immutably borrow a value that is currently foreign-owned.
>      type Borrowed<'a>;
> @@ -39,18 +35,20 @@ pub unsafe trait ForeignOwnable: Sized {
>  
>      /// Converts a Rust-owned object to a foreign-owned one.
>      ///
> +    /// The foreign representation is a pointer to void. Aside from the guarantees listed below,

I feel like this reads better:

s/guarantees/ones/

> +    /// there are no other guarantees for this pointer. For example, it might be invalid, dangling

We should also mention that it could be null. (or is that assumption
wrong?)

---
Cheers,
Benno

> +    /// 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.
> +    /// - Minimum alignment of returned pointer is [`Self::FOREIGN_ALIGN`].
>      ///
>      /// [`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 Self::PointedTo;
> +    fn into_foreign(self) -> *mut crate::ffi::c_void;
>  
>      /// Converts a foreign-owned object back to a Rust-owned one.
>      ///

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Tue Jun 10, 2025 at 1:30 PM CEST, Andreas Hindborg wrote:
>> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> index 22985b6f6982..0ccef6b5a20a 100644
>> --- a/rust/kernel/types.rs
>> +++ b/rust/kernel/types.rs
>> @@ -21,15 +21,11 @@
>>  ///
>>  /// # 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
>> +/// Implementers must ensure that [`Self::into_foreign`] returns pointers aligned to
>> +/// [`Self::FOREIGN_ALIGN`].
>>  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;
>> +    /// The alignment of pointers returned by `into_foreign`.
>> +    const FOREIGN_ALIGN: usize;
>>
>>      /// Type used to immutably borrow a value that is currently foreign-owned.
>>      type Borrowed<'a>;
>> @@ -39,18 +35,20 @@ pub unsafe trait ForeignOwnable: Sized {
>>
>>      /// Converts a Rust-owned object to a foreign-owned one.
>>      ///
>> +    /// The foreign representation is a pointer to void. Aside from the guarantees listed below,
>
> I feel like this reads better:
>
> s/guarantees/ones/
>
>> +    /// there are no other guarantees for this pointer. For example, it might be invalid, dangling
>
> We should also mention that it could be null. (or is that assumption
> wrong?)

It is probably not going to be null, but it is allowed to. I can add it.

The list does not claim to be exhaustive, and a null pointer is just a
special case of an invalid pointer.


Best regards,
Andreas Hindborg

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Alice Ryhl]

On Tue, Jun 10, 2025 at 4:10 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Benno Lossin" <lossin@kernel.org> writes:
>
> > On Tue Jun 10, 2025 at 1:30 PM CEST, Andreas Hindborg wrote:
> >> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> >> index 22985b6f6982..0ccef6b5a20a 100644
> >> --- a/rust/kernel/types.rs
> >> +++ b/rust/kernel/types.rs
> >> @@ -21,15 +21,11 @@
> >>  ///
> >>  /// # 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
> >> +/// Implementers must ensure that [`Self::into_foreign`] returns pointers aligned to
> >> +/// [`Self::FOREIGN_ALIGN`].
> >>  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;
> >> +    /// The alignment of pointers returned by `into_foreign`.
> >> +    const FOREIGN_ALIGN: usize;
> >>
> >>      /// Type used to immutably borrow a value that is currently foreign-owned.
> >>      type Borrowed<'a>;
> >> @@ -39,18 +35,20 @@ pub unsafe trait ForeignOwnable: Sized {
> >>
> >>      /// Converts a Rust-owned object to a foreign-owned one.
> >>      ///
> >> +    /// The foreign representation is a pointer to void. Aside from the guarantees listed below,
> >
> > I feel like this reads better:
> >
> > s/guarantees/ones/
> >
> >> +    /// there are no other guarantees for this pointer. For example, it might be invalid, dangling
> >
> > We should also mention that it could be null. (or is that assumption
> > wrong?)
>
> It is probably not going to be null, but it is allowed to. I can add it.
>
> The list does not claim to be exhaustive, and a null pointer is just a
> special case of an invalid pointer.

We probably should not allow null pointers. If we do, then
try_from_foreign does not make sense.

Alice

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Benno Lossin]

On Tue Jun 10, 2025 at 4:15 PM CEST, Alice Ryhl wrote:
> On Tue, Jun 10, 2025 at 4:10 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> "Benno Lossin" <lossin@kernel.org> writes:
>>
>> > On Tue Jun 10, 2025 at 1:30 PM CEST, Andreas Hindborg wrote:
>> >> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>> >> index 22985b6f6982..0ccef6b5a20a 100644
>> >> --- a/rust/kernel/types.rs
>> >> +++ b/rust/kernel/types.rs
>> >> @@ -21,15 +21,11 @@
>> >>  ///
>> >>  /// # 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
>> >> +/// Implementers must ensure that [`Self::into_foreign`] returns pointers aligned to
>> >> +/// [`Self::FOREIGN_ALIGN`].
>> >>  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;
>> >> +    /// The alignment of pointers returned by `into_foreign`.
>> >> +    const FOREIGN_ALIGN: usize;
>> >>
>> >>      /// Type used to immutably borrow a value that is currently foreign-owned.
>> >>      type Borrowed<'a>;
>> >> @@ -39,18 +35,20 @@ pub unsafe trait ForeignOwnable: Sized {
>> >>
>> >>      /// Converts a Rust-owned object to a foreign-owned one.
>> >>      ///
>> >> +    /// The foreign representation is a pointer to void. Aside from the guarantees listed below,
>> >
>> > I feel like this reads better:
>> >
>> > s/guarantees/ones/
>> >
>> >> +    /// there are no other guarantees for this pointer. For example, it might be invalid, dangling
>> >
>> > We should also mention that it could be null. (or is that assumption
>> > wrong?)
>>
>> It is probably not going to be null, but it is allowed to. I can add it.
>>
>> The list does not claim to be exhaustive, and a null pointer is just a
>> special case of an invalid pointer.
>
> We probably should not allow null pointers. If we do, then
> try_from_foreign does not make sense.

That's a good point. Then let's add that as a safety requirement for the
trait.

---
Cheers,
Benno

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Tue Jun 10, 2025 at 4:15 PM CEST, Alice Ryhl wrote:
>> On Tue, Jun 10, 2025 at 4:10 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>
>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>
>>> > On Tue Jun 10, 2025 at 1:30 PM CEST, Andreas Hindborg wrote:
>>> >> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>> >> index 22985b6f6982..0ccef6b5a20a 100644
>>> >> --- a/rust/kernel/types.rs
>>> >> +++ b/rust/kernel/types.rs
>>> >> @@ -21,15 +21,11 @@
>>> >>  ///
>>> >>  /// # 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
>>> >> +/// Implementers must ensure that [`Self::into_foreign`] returns pointers aligned to
>>> >> +/// [`Self::FOREIGN_ALIGN`].
>>> >>  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;
>>> >> +    /// The alignment of pointers returned by `into_foreign`.
>>> >> +    const FOREIGN_ALIGN: usize;
>>> >>
>>> >>      /// Type used to immutably borrow a value that is currently foreign-owned.
>>> >>      type Borrowed<'a>;
>>> >> @@ -39,18 +35,20 @@ pub unsafe trait ForeignOwnable: Sized {
>>> >>
>>> >>      /// Converts a Rust-owned object to a foreign-owned one.
>>> >>      ///
>>> >> +    /// The foreign representation is a pointer to void. Aside from the guarantees listed below,
>>> >
>>> > I feel like this reads better:
>>> >
>>> > s/guarantees/ones/
>>> >
>>> >> +    /// there are no other guarantees for this pointer. For example, it might be invalid, dangling
>>> >
>>> > We should also mention that it could be null. (or is that assumption
>>> > wrong?)
>>>
>>> It is probably not going to be null, but it is allowed to. I can add it.
>>>
>>> The list does not claim to be exhaustive, and a null pointer is just a
>>> special case of an invalid pointer.
>>
>> We probably should not allow null pointers. If we do, then
>> try_from_foreign does not make sense.
>
> That's a good point. Then let's add that as a safety requirement for the
> trait.

I disagree. It does not matter for the safety of the trait.

From the point of the user, the pointer is opaque and can be any value.
In fact, one could do a safe implementation where the returned value is
a key into some mapping structure. Probably not super fast, but the user
should not care.


Best regards,
Andreas Hindborg

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Benno Lossin]

On Wed Jun 11, 2025 at 12:43 PM CEST, Andreas Hindborg wrote:
> "Benno Lossin" <lossin@kernel.org> writes:
>
>> On Tue Jun 10, 2025 at 4:15 PM CEST, Alice Ryhl wrote:
>>> On Tue, Jun 10, 2025 at 4:10 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>
>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>
>>>> > On Tue Jun 10, 2025 at 1:30 PM CEST, Andreas Hindborg wrote:
>>>> >> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>>> >> index 22985b6f6982..0ccef6b5a20a 100644
>>>> >> --- a/rust/kernel/types.rs
>>>> >> +++ b/rust/kernel/types.rs
>>>> >> @@ -21,15 +21,11 @@
>>>> >>  ///
>>>> >>  /// # 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
>>>> >> +/// Implementers must ensure that [`Self::into_foreign`] returns pointers aligned to
>>>> >> +/// [`Self::FOREIGN_ALIGN`].
>>>> >>  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;
>>>> >> +    /// The alignment of pointers returned by `into_foreign`.
>>>> >> +    const FOREIGN_ALIGN: usize;
>>>> >>
>>>> >>      /// Type used to immutably borrow a value that is currently foreign-owned.
>>>> >>      type Borrowed<'a>;
>>>> >> @@ -39,18 +35,20 @@ pub unsafe trait ForeignOwnable: Sized {
>>>> >>
>>>> >>      /// Converts a Rust-owned object to a foreign-owned one.
>>>> >>      ///
>>>> >> +    /// The foreign representation is a pointer to void. Aside from the guarantees listed below,
>>>> >
>>>> > I feel like this reads better:
>>>> >
>>>> > s/guarantees/ones/
>>>> >
>>>> >> +    /// there are no other guarantees for this pointer. For example, it might be invalid, dangling
>>>> >
>>>> > We should also mention that it could be null. (or is that assumption
>>>> > wrong?)
>>>>
>>>> It is probably not going to be null, but it is allowed to. I can add it.
>>>>
>>>> The list does not claim to be exhaustive, and a null pointer is just a
>>>> special case of an invalid pointer.
>>>
>>> We probably should not allow null pointers. If we do, then
>>> try_from_foreign does not make sense.
>>
>> That's a good point. Then let's add that as a safety requirement for the
>> trait.
>
> I disagree. It does not matter for the safety of the trait.
>
> From the point of the user, the pointer is opaque and can be any value.
> In fact, one could do a safe implementation where the returned value is
> a key into some mapping structure. Probably not super fast, but the user
> should not care.

Then we'll have to remove `try_from_foreign`.

---
Cheers,
Benno

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Andreas Hindborg]

"Benno Lossin" <lossin@kernel.org> writes:

> On Wed Jun 11, 2025 at 12:43 PM CEST, Andreas Hindborg wrote:
>> "Benno Lossin" <lossin@kernel.org> writes:
>>
>>> On Tue Jun 10, 2025 at 4:15 PM CEST, Alice Ryhl wrote:
>>>> On Tue, Jun 10, 2025 at 4:10 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>>>>
>>>>> "Benno Lossin" <lossin@kernel.org> writes:
>>>>>
>>>>> > On Tue Jun 10, 2025 at 1:30 PM CEST, Andreas Hindborg wrote:
>>>>> >> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
>>>>> >> index 22985b6f6982..0ccef6b5a20a 100644
>>>>> >> --- a/rust/kernel/types.rs
>>>>> >> +++ b/rust/kernel/types.rs
>>>>> >> @@ -21,15 +21,11 @@
>>>>> >>  ///
>>>>> >>  /// # 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
>>>>> >> +/// Implementers must ensure that [`Self::into_foreign`] returns pointers aligned to
>>>>> >> +/// [`Self::FOREIGN_ALIGN`].
>>>>> >>  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;
>>>>> >> +    /// The alignment of pointers returned by `into_foreign`.
>>>>> >> +    const FOREIGN_ALIGN: usize;
>>>>> >>
>>>>> >>      /// Type used to immutably borrow a value that is currently foreign-owned.
>>>>> >>      type Borrowed<'a>;
>>>>> >> @@ -39,18 +35,20 @@ pub unsafe trait ForeignOwnable: Sized {
>>>>> >>
>>>>> >>      /// Converts a Rust-owned object to a foreign-owned one.
>>>>> >>      ///
>>>>> >> +    /// The foreign representation is a pointer to void. Aside from the guarantees listed below,
>>>>> >
>>>>> > I feel like this reads better:
>>>>> >
>>>>> > s/guarantees/ones/
>>>>> >
>>>>> >> +    /// there are no other guarantees for this pointer. For example, it might be invalid, dangling
>>>>> >
>>>>> > We should also mention that it could be null. (or is that assumption
>>>>> > wrong?)
>>>>>
>>>>> It is probably not going to be null, but it is allowed to. I can add it.
>>>>>
>>>>> The list does not claim to be exhaustive, and a null pointer is just a
>>>>> special case of an invalid pointer.
>>>>
>>>> We probably should not allow null pointers. If we do, then
>>>> try_from_foreign does not make sense.
>>>
>>> That's a good point. Then let's add that as a safety requirement for the
>>> trait.
>>
>> I disagree. It does not matter for the safety of the trait.
>>
>> From the point of the user, the pointer is opaque and can be any value.
>> In fact, one could do a safe implementation where the returned value is
>> a key into some mapping structure. Probably not super fast, but the user
>> should not care.
>
> Then we'll have to remove `try_from_foreign`.

Oh, I see. OK, it is a safety requirement. Should I just add it to this patch?


Best regards,
Andreas Hindborg

Re: [PATCH v2] rust: types: add FOREIGN_ALIGN to ForeignOwnable

[Alice Ryhl]

On Wed, Jun 11, 2025 at 2:14 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>
> "Benno Lossin" <lossin@kernel.org> writes:
>
> > On Wed Jun 11, 2025 at 12:43 PM CEST, Andreas Hindborg wrote:
> >> "Benno Lossin" <lossin@kernel.org> writes:
> >>
> >>> On Tue Jun 10, 2025 at 4:15 PM CEST, Alice Ryhl wrote:
> >>>> On Tue, Jun 10, 2025 at 4:10 PM Andreas Hindborg <a.hindborg@kernel.org> wrote:
> >>>>>
> >>>>> "Benno Lossin" <lossin@kernel.org> writes:
> >>>>>
> >>>>> > On Tue Jun 10, 2025 at 1:30 PM CEST, Andreas Hindborg wrote:
> >>>>> >> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> >>>>> >> index 22985b6f6982..0ccef6b5a20a 100644
> >>>>> >> --- a/rust/kernel/types.rs
> >>>>> >> +++ b/rust/kernel/types.rs
> >>>>> >> @@ -21,15 +21,11 @@
> >>>>> >>  ///
> >>>>> >>  /// # 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
> >>>>> >> +/// Implementers must ensure that [`Self::into_foreign`] returns pointers aligned to
> >>>>> >> +/// [`Self::FOREIGN_ALIGN`].
> >>>>> >>  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;
> >>>>> >> +    /// The alignment of pointers returned by `into_foreign`.
> >>>>> >> +    const FOREIGN_ALIGN: usize;
> >>>>> >>
> >>>>> >>      /// Type used to immutably borrow a value that is currently foreign-owned.
> >>>>> >>      type Borrowed<'a>;
> >>>>> >> @@ -39,18 +35,20 @@ pub unsafe trait ForeignOwnable: Sized {
> >>>>> >>
> >>>>> >>      /// Converts a Rust-owned object to a foreign-owned one.
> >>>>> >>      ///
> >>>>> >> +    /// The foreign representation is a pointer to void. Aside from the guarantees listed below,
> >>>>> >
> >>>>> > I feel like this reads better:
> >>>>> >
> >>>>> > s/guarantees/ones/
> >>>>> >
> >>>>> >> +    /// there are no other guarantees for this pointer. For example, it might be invalid, dangling
> >>>>> >
> >>>>> > We should also mention that it could be null. (or is that assumption
> >>>>> > wrong?)
> >>>>>
> >>>>> It is probably not going to be null, but it is allowed to. I can add it.
> >>>>>
> >>>>> The list does not claim to be exhaustive, and a null pointer is just a
> >>>>> special case of an invalid pointer.
> >>>>
> >>>> We probably should not allow null pointers. If we do, then
> >>>> try_from_foreign does not make sense.
> >>>
> >>> That's a good point. Then let's add that as a safety requirement for the
> >>> trait.
> >>
> >> I disagree. It does not matter for the safety of the trait.
> >>
> >> From the point of the user, the pointer is opaque and can be any value.
> >> In fact, one could do a safe implementation where the returned value is
> >> a key into some mapping structure. Probably not super fast, but the user
> >> should not care.
> >
> > Then we'll have to remove `try_from_foreign`.
>
> Oh, I see. OK, it is a safety requirement. Should I just add it to this patch?

Sure, this patch is fine.