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.
