[v16,2/4] rust: types: add `ForeignOwnable::PointedTo`

Commit Message

Tamir Duberstein Feb. 7, 2025, 1:58 p.m. UTC

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: Alice Ryhl <[e-mail]>
Reviewed-by: Andreas Hindborg <[e-mail]>
Reviewed-by: Fiona Behrens <[e-mail]>
Signed-off-by: Tamir Duberstein <[e-mail]>
---
 rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
 rust/kernel/miscdevice.rs |  7 ++++++-
 rust/kernel/pci.rs        |  2 ++
 rust/kernel/platform.rs   |  2 ++
 rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
 rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
 6 files changed, 73 insertions(+), 43 deletions(-)

Comments

Benno Lossin Feb. 17, 2025, 1:39 a.m. UTC | #1

On 07.02.25 14:58, 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: Alice Ryhl <[e-mail]>
> Reviewed-by: Andreas Hindborg <[e-mail]>
> Reviewed-by: Fiona Behrens <[e-mail]>
> Signed-off-by: Tamir Duberstein <[e-mail]>
> ---
>  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
>  rust/kernel/miscdevice.rs |  7 ++++++-
>  rust/kernel/pci.rs        |  2 ++
>  rust/kernel/platform.rs   |  2 ++
>  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
>  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
>  6 files changed, 73 insertions(+), 43 deletions(-)

When compiling this (on top of rust-next), I get the following error:

    error[E0308]: mismatched types
       --> rust/kernel/miscdevice.rs:300:62
        |
    300 |     let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
        |                           ---------------------------------- ^^^^^^^ expected `*mut <<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`, found `*mut c_void`
        |                           |
        |                           arguments to this function are incorrect
        |
        = note: expected raw pointer `*mut <<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`
                   found raw pointer `*mut c_void`
        = help: consider constraining the associated type `<<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo` to `c_void` or calling a method that returns `<<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`
        = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
    note: associated function defined here
       --> rust/kernel/types.rs:98:15
        |
    98  |     unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Self::Borrowed<'a>;
        |               ^^^^^^
    
    error: aborting due to 1 previous error

Can anyone reproduce?

---
Cheers,
Benno

Tamir Duberstein Feb. 17, 2025, 1:58 a.m. UTC | #2

On Sun, Feb 16, 2025 at 8:39 PM Benno Lossin <[e-mail]> wrote:
>
> On 07.02.25 14:58, 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: Alice Ryhl <[e-mail]>
> > Reviewed-by: Andreas Hindborg <[e-mail]>
> > Reviewed-by: Fiona Behrens <[e-mail]>
> > Signed-off-by: Tamir Duberstein <[e-mail]>
> > ---
> >  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
> >  rust/kernel/miscdevice.rs |  7 ++++++-
> >  rust/kernel/pci.rs        |  2 ++
> >  rust/kernel/platform.rs   |  2 ++
> >  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
> >  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
> >  6 files changed, 73 insertions(+), 43 deletions(-)
>
> When compiling this (on top of rust-next), I get the following error:
>
>     error[E0308]: mismatched types
>        --> rust/kernel/miscdevice.rs:300:62
>         |
>     300 |     let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) };
>         |                           ---------------------------------- ^^^^^^^ expected `*mut <<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`, found `*mut c_void`
>         |                           |
>         |                           arguments to this function are incorrect
>         |
>         = note: expected raw pointer `*mut <<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`
>                    found raw pointer `*mut c_void`
>         = help: consider constraining the associated type `<<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo` to `c_void` or calling a method that returns `<<T as MiscDevice>::Ptr as ForeignOwnable>::PointedTo`
>         = note: for more information, visit https://doc.rust-lang.org/book/ch19-03-advanced-traits.html
>     note: associated function defined here
>        --> rust/kernel/types.rs:98:15
>         |
>     98  |     unsafe fn borrow<'a>(ptr: *mut Self::PointedTo) -> Self::Borrowed<'a>;
>         |               ^^^^^^
>
>     error: aborting due to 1 previous error
>
> Can anyone reproduce?
>
> ---
> Cheers,
> Benno
>

Looks like this code is behind #[cfg(CONFIG_COMPAT)] and I missed
updating it. It is fixed by

diff --git a/rust/kernel/miscdevice.rs b/rust/kernel/miscdevice.rs
index f1a081dd64c7..004dc87f441f 100644
--- a/rust/kernel/miscdevice.rs
+++ b/rust/kernel/miscdevice.rs
@@ -296,6 +296,7 @@ impl<T: MiscDevice> VtableHelper<T> {
 ) -> c_long {
     // SAFETY: The compat 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) };

I'll include that in the next version.

Danilo Krummrich Feb. 17, 2025, 12:12 p.m. UTC | #3

On Fri, Feb 07, 2025 at 08:58:25AM -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: Alice Ryhl <[e-mail]>
> Reviewed-by: Andreas Hindborg <[e-mail]>
> Reviewed-by: Fiona Behrens <[e-mail]>

I know that Andreas also asked you to pick up the RBs from [1], but - without
speaking for any of the people above - given that you changed this commit after
you received all those RBs you should also consider dropping them. Especially,
since you do not mention the changes you did for this commit in the version
history.

Just to be clear, often it is also fine to keep tags for minor changes, but then
you should make people aware of them in the version history, such that they get
the chance to double check.

[1] https://lore.kernel.org/rust-for-linux/[e-mail]/

> Signed-off-by: Tamir Duberstein <[e-mail]>
> ---
>  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
>  rust/kernel/miscdevice.rs |  7 ++++++-
>  rust/kernel/pci.rs        |  2 ++
>  rust/kernel/platform.rs   |  2 ++
>  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
>  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
>  6 files changed, 73 insertions(+), 43 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();

I still think that it's unnecessary to factor this out, you can just do it
inline as you did in previous versions of this patch and as this code did
before.

>  
>      // 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 6c3bc14b42ad..eb25fabbff9c 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
>          match T::probe(&mut pdev, info) {
>              Ok(data) => {
>                  let data = data.into_foreign();
> +                let data = data.cast();

Same here and below, see also [2].

I understand you like this style and I'm not saying it's wrong or forbidden and
for code that you maintain such nits are entirely up to you as far as I'm
concerned.

But I also don't think there is a necessity to convert things to your preference
wherever you touch existing code.

I already explicitly asked you not to do so in [3] and yet you did so while
keeping my ACK. :(

(Only saying the latter for reference, no need to send a new version of [3],
otherwise I would have replied.)

[2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
[3] https://lore.kernel.org/rust-for-linux/[e-mail]/

>                  // 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`.
> @@ -88,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 dea104563fa9..53764cb7f804 100644
> --- a/rust/kernel/platform.rs
> +++ b/rust/kernel/platform.rs
> @@ -64,6 +64,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
>          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`.
> @@ -78,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

Tamir Duberstein Feb. 17, 2025, 2:02 p.m. UTC | #4

On Mon, Feb 17, 2025 at 7:13 AM Danilo Krummrich <[e-mail]> wrote:
>
> On Fri, Feb 07, 2025 at 08:58:25AM -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: Alice Ryhl <[e-mail]>
> > Reviewed-by: Andreas Hindborg <[e-mail]>
> > Reviewed-by: Fiona Behrens <[e-mail]>
>
> I know that Andreas also asked you to pick up the RBs from [1], but - without
> speaking for any of the people above - given that you changed this commit after
> you received all those RBs you should also consider dropping them. Especially,
> since you do not mention the changes you did for this commit in the version
> history.
>
> Just to be clear, often it is also fine to keep tags for minor changes, but then
> you should make people aware of them in the version history, such that they get
> the chance to double check.

I will drop their RBs.

> [1] https://lore.kernel.org/rust-for-linux/[e-mail]/
>
> > Signed-off-by: Tamir Duberstein <[e-mail]>
> > ---
> >  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
> >  rust/kernel/miscdevice.rs |  7 ++++++-
> >  rust/kernel/pci.rs        |  2 ++
> >  rust/kernel/platform.rs   |  2 ++
> >  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
> >  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
> >  6 files changed, 73 insertions(+), 43 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();
>
> I still think that it's unnecessary to factor this out, you can just do it
> inline as you did in previous versions of this patch and as this code did
> before.

I will do as you ask here.

>
> >
> >      // 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 6c3bc14b42ad..eb25fabbff9c 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
> >          match T::probe(&mut pdev, info) {
> >              Ok(data) => {
> >                  let data = data.into_foreign();
> > +                let data = data.cast();
>
> Same here and below, see also [2].

You're the maintainer, so I'll do what you ask here as well. I did it
this way because it avoids shadowing the git history with this change,
which I thought was the dominant preference.

> I understand you like this style and I'm not saying it's wrong or forbidden and
> for code that you maintain such nits are entirely up to you as far as I'm
> concerned.
>
> But I also don't think there is a necessity to convert things to your preference
> wherever you touch existing code.

This isn't a conversion, it's a choice made specifically to avoid
touching code that doesn't need to be touched (in this instance).

> I already explicitly asked you not to do so in [3] and yet you did so while
> keeping my ACK. :(
>
> (Only saying the latter for reference, no need to send a new version of [3],
> otherwise I would have replied.)
>
> [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
> [3] https://lore.kernel.org/rust-for-linux/[e-mail]/

I will drop [2] and leave the `as _` casts in place to minimize
controversy here.

> >                  // 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`.
> > @@ -88,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 dea104563fa9..53764cb7f804 100644
> > --- a/rust/kernel/platform.rs
> > +++ b/rust/kernel/platform.rs
> > @@ -64,6 +64,7 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
> >          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`.
> > @@ -78,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
>

Danilo Krummrich Feb. 17, 2025, 2:15 p.m. UTC | #5

On Mon, Feb 17, 2025 at 09:02:12AM -0500, Tamir Duberstein wrote:
> > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > > index 6c3bc14b42ad..eb25fabbff9c 100644
> > > --- a/rust/kernel/pci.rs
> > > +++ b/rust/kernel/pci.rs
> > > @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
> > >          match T::probe(&mut pdev, info) {
> > >              Ok(data) => {
> > >                  let data = data.into_foreign();
> > > +                let data = data.cast();
> >
> > Same here and below, see also [2].
> 
> You're the maintainer,

This isn't true. I'm the original author, but I'm not an official maintainer of
this code. :)

> so I'll do what you ask here as well. I did it
> this way because it avoids shadowing the git history with this change,
> which I thought was the dominant preference.

As mentioned in [2], if you do it the other way around first the "rust: types:
add `ForeignOwnable::PointedTo`" patch and then the conversion to cast() it's
even cleaner and less code to change.

> 
> > I understand you like this style and I'm not saying it's wrong or forbidden and
> > for code that you maintain such nits are entirely up to you as far as I'm
> > concerned.
> >
> > But I also don't think there is a necessity to convert things to your preference
> > wherever you touch existing code.
> 
> This isn't a conversion, it's a choice made specifically to avoid
> touching code that doesn't need to be touched (in this instance).

See above.

> 
> > I already explicitly asked you not to do so in [3] and yet you did so while
> > keeping my ACK. :(
> >
> > (Only saying the latter for reference, no need to send a new version of [3],
> > otherwise I would have replied.)
> >
> > [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
> > [3] https://lore.kernel.org/rust-for-linux/[e-mail]/
> 
> I will drop [2] and leave the `as _` casts in place to minimize
> controversy here.

As mentioned I think the conversion to cast() is great, just do it after this
one and keep it a single line -- no controversy. :)

Tamir Duberstein Feb. 17, 2025, 2:21 p.m. UTC | #6

On Mon, Feb 17, 2025 at 9:15 AM Danilo Krummrich <[e-mail]> wrote:
>
> On Mon, Feb 17, 2025 at 09:02:12AM -0500, Tamir Duberstein wrote:
> > > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > > > index 6c3bc14b42ad..eb25fabbff9c 100644
> > > > --- a/rust/kernel/pci.rs
> > > > +++ b/rust/kernel/pci.rs
> > > > @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
> > > >          match T::probe(&mut pdev, info) {
> > > >              Ok(data) => {
> > > >                  let data = data.into_foreign();
> > > > +                let data = data.cast();
> > >
> > > Same here and below, see also [2].
> >
> > You're the maintainer,
>
> This isn't true. I'm the original author, but I'm not an official maintainer of
> this code. :)
>
> > so I'll do what you ask here as well. I did it
> > this way because it avoids shadowing the git history with this change,
> > which I thought was the dominant preference.
>
> As mentioned in [2], if you do it the other way around first the "rust: types:
> add `ForeignOwnable::PointedTo`" patch and then the conversion to cast() it's
> even cleaner and less code to change.

This is true for the two instances of `as _`, but not for all the
other instances where currently there's no cast, but one is now
needed.

> >
> > > I understand you like this style and I'm not saying it's wrong or forbidden and
> > > for code that you maintain such nits are entirely up to you as far as I'm
> > > concerned.
> > >
> > > But I also don't think there is a necessity to convert things to your preference
> > > wherever you touch existing code.
> >
> > This isn't a conversion, it's a choice made specifically to avoid
> > touching code that doesn't need to be touched (in this instance).
>
> See above.

This doesn't address my point. I claim that

@@ -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) };

is a better diff than

@@ -245,7 +245,7 @@ impl<T: MiscDevice> VtableHelper<T> {
     file: *mut bindings::file,
 ) -> c_int {
     // SAFETY: The release call of a file owns the private data.
-    let private = unsafe { (*file).private_data };
+    let private = unsafe { (*file).private_data }.cast();
     // SAFETY: The release call of a file owns the private data.
     let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };

because it doesn't acquire the git blame on the existing line.

> >
> > > I already explicitly asked you not to do so in [3] and yet you did so while
> > > keeping my ACK. :(
> > >
> > > (Only saying the latter for reference, no need to send a new version of [3],
> > > otherwise I would have replied.)
> > >
> > > [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
> > > [3] https://lore.kernel.org/rust-for-linux/[e-mail]/
> >
> > I will drop [2] and leave the `as _` casts in place to minimize
> > controversy here.
>
> As mentioned I think the conversion to cast() is great, just do it after this
> one and keep it a single line -- no controversy. :)

The code compiles either way, so I'll leave it untouched rather than
risk being scolded for sneaking unrelated changes.

Danilo Krummrich Feb. 17, 2025, 2:37 p.m. UTC | #7

On Mon, Feb 17, 2025 at 09:21:00AM -0500, Tamir Duberstein wrote:
> On Mon, Feb 17, 2025 at 9:15 AM Danilo Krummrich <[e-mail]> wrote:
> >
> > On Mon, Feb 17, 2025 at 09:02:12AM -0500, Tamir Duberstein wrote:
> > > > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > > > > index 6c3bc14b42ad..eb25fabbff9c 100644
> > > > > --- a/rust/kernel/pci.rs
> > > > > +++ b/rust/kernel/pci.rs
> > > > > @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
> > > > >          match T::probe(&mut pdev, info) {
> > > > >              Ok(data) => {
> > > > >                  let data = data.into_foreign();
> > > > > +                let data = data.cast();
> > > >
> > > > Same here and below, see also [2].
> > >
> > > You're the maintainer,
> >
> > This isn't true. I'm the original author, but I'm not an official maintainer of
> > this code. :)
> >
> > > so I'll do what you ask here as well. I did it
> > > this way because it avoids shadowing the git history with this change,
> > > which I thought was the dominant preference.
> >
> > As mentioned in [2], if you do it the other way around first the "rust: types:
> > add `ForeignOwnable::PointedTo`" patch and then the conversion to cast() it's
> > even cleaner and less code to change.
> 
> This is true for the two instances of `as _`,

Yes, those are the ones I talk about.

> but not for all the
> other instances where currently there's no cast, but one is now
> needed.
> 
> > >
> > > > I understand you like this style and I'm not saying it's wrong or forbidden and
> > > > for code that you maintain such nits are entirely up to you as far as I'm
> > > > concerned.
> > > >
> > > > But I also don't think there is a necessity to convert things to your preference
> > > > wherever you touch existing code.
> > >
> > > This isn't a conversion, it's a choice made specifically to avoid
> > > touching code that doesn't need to be touched (in this instance).
> >
> > See above.
> 
> This doesn't address my point. I claim that
> 
> @@ -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) };
> 
> is a better diff than
> 
> @@ -245,7 +245,7 @@ impl<T: MiscDevice> VtableHelper<T> {
>      file: *mut bindings::file,
>  ) -> c_int {
>      // SAFETY: The release call of a file owns the private data.
> -    let private = unsafe { (*file).private_data };
> +    let private = unsafe { (*file).private_data }.cast();
>      // SAFETY: The release call of a file owns the private data.
>      let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
> 
> because it doesn't acquire the git blame on the existing line.

I disagree with the *rationale*, because it would also mean that if I have

  let result = a + b;

and it turns out that we're off by one later on, it'd be reasonable to change it
to

  let result = a - b;
  let result = result + 1;

in order to not acquire the git blame of the existing line.

> 
> > >
> > > > I already explicitly asked you not to do so in [3] and yet you did so while
> > > > keeping my ACK. :(
> > > >
> > > > (Only saying the latter for reference, no need to send a new version of [3],
> > > > otherwise I would have replied.)
> > > >
> > > > [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
> > > > [3] https://lore.kernel.org/rust-for-linux/[e-mail]/
> > >
> > > I will drop [2] and leave the `as _` casts in place to minimize
> > > controversy here.
> >
> > As mentioned I think the conversion to cast() is great, just do it after this
> > one and keep it a single line -- no controversy. :)
> 
> The code compiles either way, so I'll leave it untouched rather than
> risk being scolded for sneaking unrelated changes.

Again, I never did that, but as already mentioned if it came across this way,
please consider that I tell you now, that it wasn't meant to be.

You're free to do the change (I encourage that), but that's of course up to you.

Subsequently, I kindly ask you though to abstain from saying that I accused you
of something or do scold you. Thanks!

Tamir Duberstein Feb. 17, 2025, 2:47 p.m. UTC | #8

On Mon, Feb 17, 2025 at 9:37 AM Danilo Krummrich <[e-mail]> wrote:
>
> On Mon, Feb 17, 2025 at 09:21:00AM -0500, Tamir Duberstein wrote:
> > On Mon, Feb 17, 2025 at 9:15 AM Danilo Krummrich <[e-mail]> wrote:
> > >
> > > On Mon, Feb 17, 2025 at 09:02:12AM -0500, Tamir Duberstein wrote:
> > > > > > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> > > > > > index 6c3bc14b42ad..eb25fabbff9c 100644
> > > > > > --- a/rust/kernel/pci.rs
> > > > > > +++ b/rust/kernel/pci.rs
> > > > > > @@ -73,6 +73,7 @@ extern "C" fn probe_callback(
> > > > > >          match T::probe(&mut pdev, info) {
> > > > > >              Ok(data) => {
> > > > > >                  let data = data.into_foreign();
> > > > > > +                let data = data.cast();
> > > > >
> > > > > Same here and below, see also [2].
> > > >
> > > > You're the maintainer,
> > >
> > > This isn't true. I'm the original author, but I'm not an official maintainer of
> > > this code. :)
> > >
> > > > so I'll do what you ask here as well. I did it
> > > > this way because it avoids shadowing the git history with this change,
> > > > which I thought was the dominant preference.
> > >
> > > As mentioned in [2], if you do it the other way around first the "rust: types:
> > > add `ForeignOwnable::PointedTo`" patch and then the conversion to cast() it's
> > > even cleaner and less code to change.
> >
> > This is true for the two instances of `as _`,
>
> Yes, those are the ones I talk about.
>
> > but not for all the
> > other instances where currently there's no cast, but one is now
> > needed.
> >
> > > >
> > > > > I understand you like this style and I'm not saying it's wrong or forbidden and
> > > > > for code that you maintain such nits are entirely up to you as far as I'm
> > > > > concerned.
> > > > >
> > > > > But I also don't think there is a necessity to convert things to your preference
> > > > > wherever you touch existing code.
> > > >
> > > > This isn't a conversion, it's a choice made specifically to avoid
> > > > touching code that doesn't need to be touched (in this instance).
> > >
> > > See above.
> >
> > This doesn't address my point. I claim that
> >
> > @@ -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) };
> >
> > is a better diff than
> >
> > @@ -245,7 +245,7 @@ impl<T: MiscDevice> VtableHelper<T> {
> >      file: *mut bindings::file,
> >  ) -> c_int {
> >      // SAFETY: The release call of a file owns the private data.
> > -    let private = unsafe { (*file).private_data };
> > +    let private = unsafe { (*file).private_data }.cast();
> >      // SAFETY: The release call of a file owns the private data.
> >      let ptr = unsafe { <T::Ptr as ForeignOwnable>::from_foreign(private) };
> >
> > because it doesn't acquire the git blame on the existing line.
>
> I disagree with the *rationale*, because it would also mean that if I have
>
>   let result = a + b;
>
> and it turns out that we're off by one later on, it'd be reasonable to change it
> to
>
>   let result = a - b;
>   let result = result + 1;
>
> in order to not acquire the git blame of the existing line.

Like anything, it depends. If something changes from being 0-indexed
to 1-indexed then I'd say what you have there is perfectly reasonable:
the 1-bias is logically separate from `a - b`. That's a fine analogy
for what's happening in this patch.

> >
> > > >
> > > > > I already explicitly asked you not to do so in [3] and yet you did so while
> > > > > keeping my ACK. :(
> > > > >
> > > > > (Only saying the latter for reference, no need to send a new version of [3],
> > > > > otherwise I would have replied.)
> > > > >
> > > > > [2] https://lore.kernel.org/rust-for-linux/Z7MYNQgo28sr_4RS@cassiopeiae/
> > > > > [3] https://lore.kernel.org/rust-for-linux/[e-mail]/
> > > >
> > > > I will drop [2] and leave the `as _` casts in place to minimize
> > > > controversy here.
> > >
> > > As mentioned I think the conversion to cast() is great, just do it after this
> > > one and keep it a single line -- no controversy. :)
> >
> > The code compiles either way, so I'll leave it untouched rather than
> > risk being scolded for sneaking unrelated changes.
>
> Again, I never did that, but as already mentioned if it came across this way,
> please consider that I tell you now, that it wasn't meant to be.

Wasn't my intention to imply that this was something you did. It was
meant as a general observation.

> You're free to do the change (I encourage that), but that's of course up to you.

I'll create a "good first issue" for it in the RfL repository.

> Subsequently, I kindly ask you though to abstain from saying that I accused you
> of something or do scold you. Thanks!

Certainly. I'll point out, as you did, that I never said that.

Danilo Krummrich Feb. 17, 2025, 2:51 p.m. UTC | #9

On Mon, Feb 17, 2025 at 09:47:14AM -0500, Tamir Duberstein wrote:
> On Mon, Feb 17, 2025 at 9:37 AM Danilo Krummrich <[e-mail]> wrote:
> > You're free to do the change (I encourage that), but that's of course up to you.
> 
> I'll create a "good first issue" for it in the RfL repository.

That's a good idea -- thanks.

Tamir Duberstein Feb. 17, 2025, 3:50 p.m. UTC | #10

On Mon, Feb 17, 2025 at 9:52 AM Danilo Krummrich <[e-mail]> wrote:
>
> On Mon, Feb 17, 2025 at 09:47:14AM -0500, Tamir Duberstein wrote:
> > On Mon, Feb 17, 2025 at 9:37 AM Danilo Krummrich <[e-mail]> wrote:
> > > You're free to do the change (I encourage that), but that's of course up to you.
> >
> > I'll create a "good first issue" for it in the RfL repository.
>
> That's a good idea -- thanks.

What do you think about enabling clippy::ptr_as_ptr?
https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr

Do we currently enable any non-default clippy lints?

Danilo Krummrich Feb. 17, 2025, 4:35 p.m. UTC | #11

On Mon, Feb 17, 2025 at 10:50:10AM -0500, Tamir Duberstein wrote:
> On Mon, Feb 17, 2025 at 9:52 AM Danilo Krummrich <[e-mail]> wrote:
> >
> > On Mon, Feb 17, 2025 at 09:47:14AM -0500, Tamir Duberstein wrote:
> > > On Mon, Feb 17, 2025 at 9:37 AM Danilo Krummrich <[e-mail]> wrote:
> > > > You're free to do the change (I encourage that), but that's of course up to you.
> > >
> > > I'll create a "good first issue" for it in the RfL repository.
> >
> > That's a good idea -- thanks.
> 
> What do you think about enabling clippy::ptr_as_ptr?
> https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr

Seems reasonable to me, but I'm a bit out of competence here.

I know some lints are not stable and hence need to be treated with care, though
this doesn't seem to be one of them.

Additionally, I think the lint would need to be supported by all compiler
versions the kernel supports currently, which also seems to be the case (added
in: 1.51.0).

> 
> Do we currently enable any non-default clippy lints?

Yes, you can check the root Makefile [1].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Makefile?h=v6.14-rc3#n471

Miguel Ojeda Feb. 17, 2025, 5:03 p.m. UTC | #12

On Mon, Feb 17, 2025 at 3:21 PM Tamir Duberstein <[e-mail]> wrote:
>
> because it doesn't acquire the git blame on the existing line.

Hmm... What did give you the impression that we need to avoid that?

Cheers,
Miguel

Miguel Ojeda Feb. 17, 2025, 5:03 p.m. UTC | #13

On Mon, Feb 17, 2025 at 4:50 PM Tamir Duberstein <[e-mail]> wrote:
>
> What do you think about enabling clippy::ptr_as_ptr?
> https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr

`ptr_as_ptr` is one of the ones Trevor suggested back then.

It seems good to me, but there are quite a lot of instances already
around, so some cleaning would be needed.

Cheers,
Miguel

Miguel Ojeda Feb. 17, 2025, 5:03 p.m. UTC | #14

On Mon, Feb 17, 2025 at 5:35 PM Danilo Krummrich <[e-mail]> wrote:
>
> I know some lints are not stable and hence need to be treated with care, though
> this doesn't seem to be one of them.

Yeah, the "nursery" ones may be not ready for prime time.

> Additionally, I think the lint would need to be supported by all compiler
> versions the kernel supports currently, which also seems to be the case (added
> in: 1.51.0).

Yeah, though we could ignore unknown ones.

Cheers,
Miguel

Tamir Duberstein Feb. 17, 2025, 5:11 p.m. UTC | #15

On Mon, Feb 17, 2025 at 12:03 PM Miguel Ojeda
<[e-mail]> wrote:
>
> On Mon, Feb 17, 2025 at 3:21 PM Tamir Duberstein <[e-mail]> wrote:
> >
> > because it doesn't acquire the git blame on the existing line.
>
> Hmm... What did give you the impression that we need to avoid that?

Only my personal experience with git blame. The `.cast()` call itself
is boilerplate that arises from the difference in types between the
abstractions and the bindings; my opinion is that a user of git blame
is more likely to be interested in the rationale accompanying the
implementation rather than that of the type shuffle.

Tamir

Miguel Ojeda Feb. 17, 2025, 5:24 p.m. UTC | #16

On Mon, Feb 17, 2025 at 6:11 PM Tamir Duberstein <[e-mail]> wrote:
>
> Only my personal experience with git blame. The `.cast()` call itself
> is boilerplate that arises from the difference in types between the
> abstractions and the bindings; my opinion is that a user of git blame
> is more likely to be interested in the rationale accompanying the
> implementation rather than that of the type shuffle.

I understand the rationale -- what I meant to ask is if you saw that
rule in kernel documentation or similar, because I could be missing
something, but I don't think we do that in the kernel.

So if you have a change like that, please just change the line, rather
than adding new ones just for `git blame`.

Cheers,
Miguel

Miguel Ojeda Feb. 17, 2025, 5:36 p.m. UTC | #17

On Mon, Feb 17, 2025 at 6:24 PM Miguel Ojeda
<[e-mail]> wrote:
>
> I understand the rationale -- what I meant to ask is if you saw that

By the way, I don't agree with the rationale, because it sounds to me
like optimizing for `git blame` readers, while pessimizing for normal
readers.

We do a lot of `git blame` in the kernel, especially since our Git log
is quite good, but we still read the files themselves more... I can
imagine ending up with a lot of extra lines over time everywhere, it
could dissuade small fixes and so on.

Cheers,
Miguel

Tamir Duberstein Feb. 17, 2025, 5:43 p.m. UTC | #18

On Mon, Feb 17, 2025 at 12:36 PM Miguel Ojeda
<[e-mail]> wrote:
>
> On Mon, Feb 17, 2025 at 6:24 PM Miguel Ojeda
> <[e-mail]> wrote:
> >
> > I understand the rationale -- what I meant to ask is if you saw that
>
> By the way, I don't agree with the rationale, because it sounds to me
> like optimizing for `git blame` readers, while pessimizing for normal
> readers.
>
> We do a lot of `git blame` in the kernel, especially since our Git log
> is quite good, but we still read the files themselves more... I can
> imagine ending up with a lot of extra lines over time everywhere, it
> could dissuade small fixes and so on.

I almost addressed this in my original reply - I regret that I didn't.

I agree with you that optimizing for git blame while pessimizing for
normal readers is not what we should do. I don't agree that putting
boilerplate on its own line is a pessimization for the normal reader -
in my opinion it is the opposite. Trivial expressions of the form

let foo = foo.cast();

can be very easily skimmed by a reader, whereas an expression of the form

unsafe { <type as trait>::associated_type::function(foo.cast()) }

become more difficult to read with every operation that is added to it.

As always, this is just my opinion.

Miguel Ojeda Feb. 17, 2025, 6:12 p.m. UTC | #19

On Mon, Feb 17, 2025 at 6:44 PM Tamir Duberstein <[e-mail]> wrote:
>
> I agree with you that optimizing for git blame while pessimizing for
> normal readers is not what we should do. I don't agree that putting
> boilerplate on its own line is a pessimization for the normal reader -
> in my opinion it is the opposite. Trivial expressions of the form

But that is a different argument, unrelated to `git blame`, no?

What I was saying is that, if the only reason one is adding a line is
for `git blame`, then it shouldn't be done.

But, of course, if there is a different, good reason to add a line,
then it should be done.

In other words, `git blame` does not play a role here.

I mean, a reasonable person could say it should at least have a small
weight into the decision, sure. But I don't think we currently do that
and it makes decisions even more complex...

Cheers,
Miguel

Tamir Duberstein Feb. 17, 2025, 6:24 p.m. UTC | #20

On Mon, Feb 17, 2025 at 1:13 PM Miguel Ojeda
<[e-mail]> wrote:
>
> On Mon, Feb 17, 2025 at 6:44 PM Tamir Duberstein <[e-mail]> wrote:
> >
> > I agree with you that optimizing for git blame while pessimizing for
> > normal readers is not what we should do. I don't agree that putting
> > boilerplate on its own line is a pessimization for the normal reader -
> > in my opinion it is the opposite. Trivial expressions of the form
>
> But that is a different argument, unrelated to `git blame`, no?

Yes it is. I suppose I misunderstood your objection to this rationale,
along with

> So if you have a change like that, please just change the line, rather
than adding new ones just for `git blame`.

as an objection to the decision, so I was giving additional rationale.

> What I was saying is that, if the only reason one is adding a line is
> for `git blame`, then it shouldn't be done.
>
> But, of course, if there is a different, good reason to add a line,
> then it should be done.
>
> In other words, `git blame` does not play a role here.
>
> I mean, a reasonable person could say it should at least have a small
> weight into the decision, sure. But I don't think we currently do that
> and it makes decisions even more complex...

Unclear where this leaves us so I'll just go with .cast() in-line.

Andreas Hindborg Feb. 18, 2025, 8:59 a.m. UTC | #21

"Danilo Krummrich" <[e-mail]> writes:

> On Fri, Feb 07, 2025 at 08:58:25AM -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: Alice Ryhl <[e-mail]>
>> Reviewed-by: Andreas Hindborg <[e-mail]>
>> Reviewed-by: Fiona Behrens <[e-mail]>
>
> I know that Andreas also asked you to pick up the RBs from [1], but - without
> speaking for any of the people above - given that you changed this commit after
> you received all those RBs you should also consider dropping them. Especially,
> since you do not mention the changes you did for this commit in the version
> history.
>
> Just to be clear, often it is also fine to keep tags for minor changes, but then
> you should make people aware of them in the version history, such that they get
> the chance to double check.
>
> [1] https://lore.kernel.org/rust-for-linux/[e-mail]/
>

As long as the commit was not radically changed, I see no point in
dropping the commit trailers. Same policy as for dropping review tags
when issuing a new version of a series should be applied.


Best regards,
Andreas Hindborg

Patch

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 6c3bc14b42ad..eb25fabbff9c 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -73,6 +73,7 @@  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`.
@@ -88,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 dea104563fa9..53764cb7f804 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -64,6 +64,7 @@  extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
         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`.
@@ -78,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.