Re: [PATCH v7 02/16] rust: implement generic driver registration

[Gary Guo <gary@garyguo.net>]

On Thu, 19 Dec 2024 18:04:04 +0100
Danilo Krummrich <dakr@kernel.org> wrote:

> Implement the generic `Registration` type and the `RegistrationOps`
> trait.
> 
> The `Registration` structure is the common type that represents a driver
> registration and is typically bound to the lifetime of a module. However,
> it doesn't implement actual calls to the kernel's driver core to register
> drivers itself.
> 
> Instead the `RegistrationOps` trait is provided to subsystems, which have
> to implement `RegistrationOps::register` and
> `RegistrationOps::unregister`. Subsystems have to provide an
> implementation for both of those methods where the subsystem specific
> variants to register / unregister a driver have to implemented.
> 
> For instance, the PCI subsystem would call __pci_register_driver() from
> `RegistrationOps::register` and pci_unregister_driver() from
> `DrvierOps::unregister`.
> 
> Co-developed-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Hi Danilo,

I think there're soundness issues with this API, please see comments
inlined below.

Best,
Gary

> ---
>  MAINTAINERS           |   1 +
>  rust/kernel/driver.rs | 117 ++++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/lib.rs    |   1 +
>  3 files changed, 119 insertions(+)
>  create mode 100644 rust/kernel/driver.rs
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index baf0eeb9a355..2ad58ed40079 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7033,6 +7033,7 @@ F:	include/linux/kobj*
>  F:	include/linux/property.h
>  F:	lib/kobj*
>  F:	rust/kernel/device.rs
> +F:	rust/kernel/driver.rs
>  
>  DRIVERS FOR OMAP ADAPTIVE VOLTAGE SCALING (AVS)
>  M:	Nishanth Menon <nm@ti.com>
> diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
> new file mode 100644
> index 000000000000..c1957ee7bb7e
> --- /dev/null
> +++ b/rust/kernel/driver.rs
> @@ -0,0 +1,117 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Generic support for drivers of different buses (e.g., PCI, Platform, Amba, etc.).
> +//!
> +//! Each bus / subsystem is expected to implement [`RegistrationOps`], which allows drivers to
> +//! register using the [`Registration`] class.
> +
> +use crate::error::{Error, Result};
> +use crate::{init::PinInit, str::CStr, try_pin_init, types::Opaque, ThisModule};
> +use core::pin::Pin;
> +use macros::{pin_data, pinned_drop};
> +
> +/// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform,
> +/// Amba, etc.) to provide the corresponding subsystem specific implementation to register /
> +/// unregister a driver of the particular type (`RegType`).
> +///
> +/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call
> +/// `bindings::__pci_register_driver` from `RegistrationOps::register` and
> +/// `bindings::pci_unregister_driver` from `RegistrationOps::unregister`.
> +pub trait RegistrationOps {
> +    /// The type that holds information about the registration. This is typically a struct defined
> +    /// by the C portion of the kernel.
> +    type RegType: Default;
> +
> +    /// Registers a driver.
> +    ///
> +    /// On success, `reg` must remain pinned and valid until the matching call to
> +    /// [`RegistrationOps::unregister`].

This looks like an obligation for the caller, so this function should
be unsafe?

> +    fn register(
> +        reg: &Opaque<Self::RegType>,
> +        name: &'static CStr,
> +        module: &'static ThisModule,
> +    ) -> Result;
> +
> +    /// Unregisters a driver previously registered with [`RegistrationOps::register`].

Similarly this is an obligation for the caller.

> +    fn unregister(reg: &Opaque<Self::RegType>);
> +}
> +
> +/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
> +/// `bindings::pci_driver`). Therefore a [`Registration`] must be initialized with a type that
> +/// implements the [`RegistrationOps`] trait, such that the generic `T::register` and
> +/// `T::unregister` calls result in the subsystem specific registration calls.
> +///
> +///Once the `Registration` structure is dropped, the driver is unregistered.
> +#[pin_data(PinnedDrop)]
> +pub struct Registration<T: RegistrationOps> {
> +    #[pin]
> +    reg: Opaque<T::RegType>,
> +}
> +
> +// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to
> +// share references to it with multiple threads as nothing can be done.
> +unsafe impl<T: RegistrationOps> Sync for Registration<T> {}
> +
> +// SAFETY: Both registration and unregistration are implemented in C and safe to be performed from
> +// any thread, so `Registration` is `Send`.
> +unsafe impl<T: RegistrationOps> Send for Registration<T> {}
> +
> +impl<T: RegistrationOps> Registration<T> {
> +    /// Creates a new instance of the registration object.
> +    pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> {
> +        try_pin_init!(Self {
> +            reg <- Opaque::try_ffi_init(|ptr: *mut T::RegType| {
> +                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
> +                unsafe { ptr.write(T::RegType::default()) };

Any reason that this is initialised with a default, and not be up to
`T::register` to initialise?

> +
> +                // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write, and it has
> +                // just been initialised above, so it's also valid for read.

Opaque can hold uninitialised value so as long as `ptr` is not dangling
this is fine. There's no need to actually initialise.

> +                let drv = unsafe { &*(ptr as *const Opaque<T::RegType>) };
> +
> +                T::register(drv, name, module)
> +            }),
> +        })
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<T: RegistrationOps> PinnedDrop for Registration<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        T::unregister(&self.reg);
> +    }
> +}
> +
> +/// Declares a kernel module that exposes a single driver.
> +///
> +/// It is meant to be used as a helper by other subsystems so they can more easily expose their own
> +/// macros.
> +#[macro_export]

I think this is supposed to be used by other macros only? If so, please
add `#[doc(hidden)]`.

> +macro_rules! module_driver {
> +    (<$gen_type:ident>, $driver_ops:ty, { type: $type:ty, $($f:tt)* }) => {
> +        type Ops<$gen_type> = $driver_ops;
> +
> +        #[$crate::prelude::pin_data]
> +        struct DriverModule {
> +            #[pin]
> +            _driver: $crate::driver::Registration<Ops<$type>>,
> +        }
> +
> +        impl $crate::InPlaceModule for DriverModule {
> +            fn init(
> +                module: &'static $crate::ThisModule
> +            ) -> impl $crate::init::PinInit<Self, $crate::error::Error> {
> +                $crate::try_pin_init!(Self {
> +                    _driver <- $crate::driver::Registration::new(
> +                        <Self as $crate::ModuleMetadata>::NAME,
> +                        module,
> +                    ),
> +                })
> +            }
> +        }
> +
> +        $crate::prelude::module! {
> +            type: DriverModule,
> +            $($f)*
> +        }
> +    }
> +}
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index 61b82b78b915..7818407f9aac 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -35,6 +35,7 @@
>  mod build_assert;
>  pub mod cred;
>  pub mod device;
> +pub mod driver;
>  pub mod error;
>  #[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)]
>  pub mod firmware;