[PATCH 0/3] Fixes for Rust Device / Driver abstractions

[PATCH 0/3] Fixes for Rust Device / Driver abstractions

[Danilo Krummrich]

This patch series contains a few fixes for the preceding series [1] introducing
device / driver Rust abstractions.

Except for the CONFIG_PCI_MSI dependency fix, there's no functional change.

[1] https://lore.kernel.org/rust-for-linux/20241219170425.12036-1-dakr@kernel.org/

Danilo Krummrich (3):
  rust: pci: do not depend on CONFIG_PCI_MSI
  rust: io: move module entry to its correct location
  rust: driver: address soundness issue in `RegistrationOps`

 rust/kernel/driver.rs   | 25 ++++++++++++++++++++-----
 rust/kernel/lib.rs      |  6 +++---
 rust/kernel/pci.rs      |  8 +++++---
 rust/kernel/platform.rs |  8 +++++---
 4 files changed, 33 insertions(+), 14 deletions(-)


base-commit: 06e843bbbf2107463249ea6f6b1a736f5647e24a
prerequisite-patch-id: 5ce64efa11560206236db2b2f014ceaa3d2e9fa3
prerequisite-patch-id: e86a1e93ce4e9e8440be0bb641ef99def521b75f
-- 
2.47.1

[PATCH 1/3] rust: pci: do not depend on CONFIG_PCI_MSI

[Danilo Krummrich]

The PCI abstractions do not actually depend on CONFIG_PCI_MSI; it also
breaks drivers that only depend on CONFIG_PCI, hence drop it.

While at it, move the module entry to its correct location.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202501030744.4ucqC1cB-lkp@intel.com/
Fixes: 3a9c09193657 ("rust: pci: add basic PCI device / driver abstractions")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/lib.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index e59250dc6c6e..b7351057ed9c 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -58,6 +58,8 @@
 pub mod net;
 pub mod of;
 pub mod page;
+#[cfg(CONFIG_PCI)]
+pub mod pci;
 pub mod pid_namespace;
 pub mod platform;
 pub mod prelude;
@@ -84,8 +86,6 @@
 pub use bindings;
 pub mod io;
 pub use macros;
-#[cfg(all(CONFIG_PCI, CONFIG_PCI_MSI))]
-pub mod pci;
 pub use uapi;
 
 #[doc(hidden)]
-- 
2.47.1

[PATCH 2/3] rust: io: move module entry to its correct location

[Danilo Krummrich]

The module entry of `io` falsely ended up in the "use" block instead of
the "mod" block, hence move it to its correct location.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/lib.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index b7351057ed9c..b11fa08de3c0 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -48,6 +48,7 @@
 pub mod firmware;
 pub mod fs;
 pub mod init;
+pub mod io;
 pub mod ioctl;
 pub mod jump_label;
 #[cfg(CONFIG_KUNIT)]
@@ -84,7 +85,6 @@
 
 #[doc(hidden)]
 pub use bindings;
-pub mod io;
 pub use macros;
 pub use uapi;
 
-- 
2.47.1

[PATCH 3/3] rust: driver: address soundness issue in `RegistrationOps`

[Danilo Krummrich]

The `RegistrationOps` trait holds some obligations to the caller and
implementers. While being documented, the trait and the corresponding
functions haven't been marked as unsafe.

Hence, markt the trait and functions unsafe and add the corresponding
safety comments.

This patch does not include any fuctional changes.

Reported-by: Gary Guo <gary@garyguo.net>
Closes: https://lore.kernel.org/rust-for-linux/20241224195821.3b43302b.gary@garyguo.net/
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/driver.rs   | 25 ++++++++++++++++++++-----
 rust/kernel/pci.rs      |  8 +++++---
 rust/kernel/platform.rs |  8 +++++---
 3 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/rust/kernel/driver.rs b/rust/kernel/driver.rs
index c630e65098ed..2a16d5e64e6c 100644
--- a/rust/kernel/driver.rs
+++ b/rust/kernel/driver.rs
@@ -17,23 +17,35 @@
 /// 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 {
+///
+/// # Safety
+///
+/// A call to [`RegistrationOps::unregister`] for a given instance of `RegType` is only valid if a
+/// preceding call to [`RegistrationOps::register`] has been successful.
+pub unsafe 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.
     ///
+    /// # Safety
+    ///
     /// On success, `reg` must remain pinned and valid until the matching call to
     /// [`RegistrationOps::unregister`].
-    fn register(
+    unsafe fn register(
         reg: &Opaque<Self::RegType>,
         name: &'static CStr,
         module: &'static ThisModule,
     ) -> Result;
 
     /// Unregisters a driver previously registered with [`RegistrationOps::register`].
-    fn unregister(reg: &Opaque<Self::RegType>);
+    ///
+    /// # Safety
+    ///
+    /// Must only be called after a preceding successful call to [`RegistrationOps::register`] for
+    /// the same `reg`.
+    unsafe fn unregister(reg: &Opaque<Self::RegType>);
 }
 
 /// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
@@ -68,7 +80,8 @@ pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Sel
                 // just been initialised above, so it's also valid for read.
                 let drv = unsafe { &*(ptr as *const Opaque<T::RegType>) };
 
-                T::register(drv, name, module)
+                // SAFETY: `drv` is guaranteed to be pinned until `T::unregister`.
+                unsafe { T::register(drv, name, module) }
             }),
         })
     }
@@ -77,7 +90,9 @@ pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Sel
 #[pinned_drop]
 impl<T: RegistrationOps> PinnedDrop for Registration<T> {
     fn drop(self: Pin<&mut Self>) {
-        T::unregister(&self.reg);
+        // SAFETY: The existence of `self` guarantees that `self.reg` has previously been
+        // successfully registered with `T::register`
+        unsafe { T::unregister(&self.reg) };
     }
 }
 
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index d5e7f0b15303..4c98b5b9aa1e 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -23,10 +23,12 @@
 /// An adapter for the registration of PCI drivers.
 pub struct Adapter<T: Driver>(T);
 
-impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
+// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// a preceding call to `register` has been successful.
+unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
     type RegType = bindings::pci_driver;
 
-    fn register(
+    unsafe fn register(
         pdrv: &Opaque<Self::RegType>,
         name: &'static CStr,
         module: &'static ThisModule,
@@ -45,7 +47,7 @@ fn register(
         })
     }
 
-    fn unregister(pdrv: &Opaque<Self::RegType>) {
+    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
         // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
         unsafe { bindings::pci_unregister_driver(pdrv.get()) }
     }
diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs
index 03287794f9d0..50e6b0421813 100644
--- a/rust/kernel/platform.rs
+++ b/rust/kernel/platform.rs
@@ -19,10 +19,12 @@
 /// An adapter for the registration of platform drivers.
 pub struct Adapter<T: Driver>(T);
 
-impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
+// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// a preceding call to `register` has been successful.
+unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
     type RegType = bindings::platform_driver;
 
-    fn register(
+    unsafe fn register(
         pdrv: &Opaque<Self::RegType>,
         name: &'static CStr,
         module: &'static ThisModule,
@@ -44,7 +46,7 @@ fn register(
         to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) })
     }
 
-    fn unregister(pdrv: &Opaque<Self::RegType>) {
+    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
         // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
         unsafe { bindings::platform_driver_unregister(pdrv.get()) };
     }
-- 
2.47.1

Re: [PATCH 3/3] rust: driver: address soundness issue in `RegistrationOps`

[Gary Guo]

On Fri,  3 Jan 2025 17:46:03 +0100
Danilo Krummrich <dakr@kernel.org> wrote:

> The `RegistrationOps` trait holds some obligations to the caller and
> implementers. While being documented, the trait and the corresponding
> functions haven't been marked as unsafe.
> 
> Hence, markt the trait and functions unsafe and add the corresponding
> safety comments.

nit: markt -> mark

> 
> This patch does not include any fuctional changes.
> 
> Reported-by: Gary Guo <gary@garyguo.net>
> Closes: https://lore.kernel.org/rust-for-linux/20241224195821.3b43302b.gary@garyguo.net/
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/driver.rs   | 25 ++++++++++++++++++++-----
>  rust/kernel/pci.rs      |  8 +++++---
>  rust/kernel/platform.rs |  8 +++++---
>  3 files changed, 30 insertions(+), 11 deletions(-)

Re: [PATCH 1/3] rust: pci: do not depend on CONFIG_PCI_MSI

[Greg KH]

On Fri, Jan 03, 2025 at 05:46:01PM +0100, Danilo Krummrich wrote:
> The PCI abstractions do not actually depend on CONFIG_PCI_MSI; it also
> breaks drivers that only depend on CONFIG_PCI, hence drop it.
> 
> While at it, move the module entry to its correct location.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202501030744.4ucqC1cB-lkp@intel.com/
> Fixes: 3a9c09193657 ("rust: pci: add basic PCI device / driver abstractions")

Where did that git id come from?  It's
1bd8b6b2c5d38d9881d59928b986eacba40f9da8.  I'll go edit it by hand...

thanks,

greg k-h

Re: [PATCH 1/3] rust: pci: do not depend on CONFIG_PCI_MSI

[Danilo Krummrich]

On Tue, Jan 07, 2025 at 11:31:26AM +0100, Greg KH wrote:
> On Fri, Jan 03, 2025 at 05:46:01PM +0100, Danilo Krummrich wrote:
> > The PCI abstractions do not actually depend on CONFIG_PCI_MSI; it also
> > breaks drivers that only depend on CONFIG_PCI, hence drop it.
> > 
> > While at it, move the module entry to its correct location.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/oe-kbuild-all/202501030744.4ucqC1cB-lkp@intel.com/
> > Fixes: 3a9c09193657 ("rust: pci: add basic PCI device / driver abstractions")
> 
> Where did that git id come from?  It's
> 1bd8b6b2c5d38d9881d59928b986eacba40f9da8.  I'll go edit it by hand...

Oops, that's from my tree. I did not rebase to the driver-core tree, sorry.

> 
> thanks,
> 
> greg k-h