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

[Tamir Duberstein]

On Thu, Feb 6, 2025 at 11:39 AM Danilo Krummrich <dakr@xxxxxxxxxx> wrote:
>
> On Thu, Feb 06, 2025 at 11:24:43AM -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: Andreas Hindborg <a.hindborg@xxxxxxxxxx>
> > Signed-off-by: Tamir Duberstein <tamird@xxxxxxxxx>
> > ---
> >  rust/kernel/alloc/kbox.rs | 38 ++++++++++++++++++++------------------
> >  rust/kernel/miscdevice.rs |  7 ++++++-
> >  rust/kernel/pci.rs        |  5 ++++-
> >  rust/kernel/platform.rs   |  5 ++++-
> >  rust/kernel/sync/arc.rs   | 21 ++++++++++++---------
> >  rust/kernel/types.rs      | 46 +++++++++++++++++++++++++++++++---------------
> >  6 files changed, 77 insertions(+), 45 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();
> >
> >      // 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 };
>
> Why not just ptr.into_foreign().cast()?

That would work too. I was trying to move stuff out of the unsafe block.

> >
> >      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 4c98b5b9aa1e..eb25fabbff9c 100644
> > --- a/rust/kernel/pci.rs
> > +++ b/rust/kernel/pci.rs
> > @@ -72,10 +72,12 @@ 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`.
> > -                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> > +                unsafe { bindings::pci_set_drvdata(pdev.as_raw(), data) };
>
> This change isn't necessary for this patch, is it? I think it makes sense to
> replace `as _` with cast(), but this should be a separate patch then.

Sure, I can make this a separate (prequel) patch.

> >              }
> >              Err(err) => return Error::to_errno(err),
> >          }
> > @@ -87,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 50e6b0421813..53764cb7f804 100644
> > --- a/rust/kernel/platform.rs
> > +++ b/rust/kernel/platform.rs
> > @@ -63,10 +63,12 @@ extern "C" fn probe_callback(pdev: *mut bindings::platform_device) -> kernel::ff
> >          let info = <Self as driver::Adapter>::id_info(pdev.as_ref());
> >          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`.
> > -                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data.into_foreign() as _) };
> > +                unsafe { bindings::platform_set_drvdata(pdev.as_raw(), data) };
>
> Same here.

Yep. Will do.