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

[Tamir Duberstein]

On Wed, Apr 30, 2025 at 11:31 AM Gary Guo <gary@xxxxxxxxxxx> wrote:
>
> On Wed, 23 Apr 2025 09:54:37 -0400
> Tamir Duberstein <tamird@xxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Tamir Duberstein <tamird@xxxxxxxxx>
> > ---
> >  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.

Can you give an example?

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

Is your concern that this is *too restrictive*? It might be that the
minimum alignment is greater than `align_of::<PointedTo>()`, but not
less.

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

How do you determine the value? You're at quite some distance from the
allocator implementation.