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

[Danilo Krummrich <dakr@kernel.org>]

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 <aliceryhl@google.com>
> Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
> Reviewed-by: Fiona Behrens <me@kloenk.dev>

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/20250131-configfs-v1-1-87947611401c@kernel.org/

> Signed-off-by: Tamir Duberstein <tamird@gmail.com>
> ---
>  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
>  rust/kernel/miscdevice.rs |  7 ++++++-
>  rust/kernel/pci.rs        |  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/20250213-aligned-alloc-v7-1-d2a2d0be164b@gmail.com/

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