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

[Gary Guo <gary@garyguo.net>]

On Wed, 23 Apr 2025 09:54:37 -0400
Tamir Duberstein <tamird@gmail.com> 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.
> 
> Acked-by: Danilo Krummrich <dakr@kernel.org>
> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
>  rust/kernel/miscdevice.rs | 10 +++++-----
>  rust/kernel/pci.rs        |  2 +-
>  rust/kernel/platform.rs   |  2 +-
>  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
>  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
>  6 files changed, 70 insertions(+), 49 deletions(-)
> 
> diff --git a/rust/kernel/alloc/kbox.rs b/rust/kernel/alloc/kbox.rs
> index b77d32f3a58b..6aa88b01e84d 100644
> --- a/rust/kernel/alloc/kbox.rs
> +++ b/rust/kernel/alloc/kbox.rs
> @@ -360,68 +360,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;

I don't think this is the correct solution for this. The returned
pointer is supposed to opaque, and exposing this type may encourage
this is to be wrongly used.

IIUC, the only reason for this to be exposed is for XArray to be able
to check alignment. However `align_of::<PointedTo>()` is not the
minimum guaranteed alignment.

For example, if the type is allocated via kernel allocator then it can
always guarantee to be at least usize-aligned. ZST can be arbitrarily
aligned so ForeignOwnable` implementation could return a
validly-aligned pointer for XArray. Actually, I think all current
ForeignOwnable implementation can be modified to always give 4-byte
aligned pointers.

Having a const associated item indicating the minimum guaranteed
alignment for *that specific container* is the correct way IMO, not to
reason about the pointee type!

Best,
Gary

>      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 }
>      }
>  }