Address race condition with Device::drvdata()

The Driver trait describes the layout of a specific driver structure, such as struct pci_driver or struct platform_driver.

In a first step, this replaces the associated type RegType of the RegistrationOps with the Driver::DriverType associated type.

Signed-off-by Danilo Krummrich <[email protected]>

---
  rust/kernel/auxiliary.rs | 18 +++++++++++-------
  rust/kernel/driver.rs    | 40 +++++++++++++++++++++++++---------------
  rust/kernel/i2c.rs       | 18 +++++++++++-------
  rust/kernel/pci.rs       | 18 +++++++++++-------
  rust/kernel/platform.rs  | 18 +++++++++++-------
  rust/kernel/usb.rs       | 18 +++++++++++-------
  6 files changed, 80 insertions(+), 50 deletions(-)

rust/kernel/auxiliary.rs +11 -7

index 6931f8a4267f..4636b6f41195 100644
@@ -23,13 +23,17 @@
  /// An adapter for the registration of auxiliary drivers.
  pub struct Adapter<T: Driver>(T);
 
-// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// SAFETY:
+// - `bindings::auxiliary_driver` is a C type declared as `repr(C)`.
+unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
+    type DriverType = bindings::auxiliary_driver;
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` 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::auxiliary_driver;
-
      unsafe fn register(
-        adrv: &Opaque<Self::RegType>,
+        adrv: &Opaque<Self::DriverType>,
          name: &'static CStr,
          module: &'static ThisModule,
      ) -> Result {

@@ -41,14 +45,14 @@ unsafe fn register(
              (*adrv.get()).id_table = T::ID_TABLE.as_ptr();
          }
 
-        // SAFETY: `adrv` is guaranteed to be a valid `RegType`.
+        // SAFETY: `adrv` is guaranteed to be a valid `DriverType`.
          to_result(unsafe {
              bindings::__auxiliary_driver_register(adrv.get(), module.0, name.as_char_ptr())
          })
      }
 
-    unsafe fn unregister(adrv: &Opaque<Self::RegType>) {
-        // SAFETY: `adrv` is guaranteed to be a valid `RegType`.
+    unsafe fn unregister(adrv: &Opaque<Self::DriverType>) {
+        // SAFETY: `adrv` is guaranteed to be a valid `DriverType`.
          unsafe { bindings::auxiliary_driver_unregister(adrv.get()) }
      }
  }

rust/kernel/driver.rs +25 -15

index 649d06468f41..cd1d36c313e1 100644
@@ -99,23 +99,33 @@
  use core::pin::Pin;
  use pin_init::{pin_data, pinned_drop, PinInit};
 
+/// Trait describing the layout of a specific device driver.
+///
+/// This trait describes the layout of a specific driver structure, such as `struct pci_driver` or
+/// `struct platform_driver`.
+///
+/// # Safety
+///
+/// Implementors must guarantee that:
+/// - `DriverType` is `repr(C)`.
+pub unsafe trait Driver {
+    /// The specific driver type embedding a `struct device_driver`.
+    type DriverType: Default;
+}
+
  /// 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`).
+/// unregister a driver of the particular type (`DriverType`).
  ///
-/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call
+/// For instance, the PCI subsystem would set `DriverType` to `bindings::pci_driver` and call
  /// `bindings::__pci_register_driver` from `RegistrationOps::register` and
  /// `bindings::pci_unregister_driver` from `RegistrationOps::unregister`.
  ///
  /// # 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;
-
+/// A call to [`RegistrationOps::unregister`] for a given instance of `DriverType` is only valid if
+/// a preceding call to [`RegistrationOps::register`] has been successful.
+pub unsafe trait RegistrationOps: Driver {
      /// Registers a driver.
      ///
      /// # Safety

@@ -123,7 +133,7 @@ pub unsafe trait RegistrationOps {
      /// On success, `reg` must remain pinned and valid until the matching call to
      /// [`RegistrationOps::unregister`].
      unsafe fn register(
-        reg: &Opaque<Self::RegType>,
+        reg: &Opaque<Self::DriverType>,
          name: &'static CStr,
          module: &'static ThisModule,
      ) -> Result;

@@ -134,7 +144,7 @@ unsafe fn register(
      ///
      /// Must only be called after a preceding successful call to [`RegistrationOps::register`] for
      /// the same `reg`.
-    unsafe fn unregister(reg: &Opaque<Self::RegType>);
+    unsafe fn unregister(reg: &Opaque<Self::DriverType>);
  }
 
  /// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.

@@ -146,7 +156,7 @@ unsafe fn register(
  #[pin_data(PinnedDrop)]
  pub struct Registration<T: RegistrationOps> {
      #[pin]
-    reg: Opaque<T::RegType>,
+    reg: Opaque<T::DriverType>,
  }
 
  // SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to

@@ -161,13 +171,13 @@ 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| {
+            reg <- Opaque::try_ffi_init(|ptr: *mut T::DriverType| {
                  // SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
-                unsafe { ptr.write(T::RegType::default()) };
+                unsafe { ptr.write(T::DriverType::default()) };
 
                  // 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.
-                let drv = unsafe { &*(ptr as *const Opaque<T::RegType>) };
+                let drv = unsafe { &*(ptr as *const Opaque<T::DriverType>) };
 
                  // SAFETY: `drv` is guaranteed to be pinned until `T::unregister`.
                  unsafe { T::register(drv, name, module) }

rust/kernel/i2c.rs +11 -7

index 35b678b78d91..de35961c6903 100644
@@ -92,13 +92,17 @@ macro_rules! i2c_device_table {
  /// An adapter for the registration of I2C drivers.
  pub struct Adapter<T: Driver>(T);
 
-// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// SAFETY:
+// - `bindings::i2c_driver` is a C type declared as `repr(C)`.
+unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
+    type DriverType = bindings::i2c_driver;
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` 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::i2c_driver;
-
      unsafe fn register(
-        idrv: &Opaque<Self::RegType>,
+        idrv: &Opaque<Self::DriverType>,
          name: &'static CStr,
          module: &'static ThisModule,
      ) -> Result {

@@ -133,12 +137,12 @@ unsafe fn register(
              (*idrv.get()).driver.acpi_match_table = acpi_table;
          }
 
-        // SAFETY: `idrv` is guaranteed to be a valid `RegType`.
+        // SAFETY: `idrv` is guaranteed to be a valid `DriverType`.
          to_result(unsafe { bindings::i2c_register_driver(module.0, idrv.get()) })
      }
 
-    unsafe fn unregister(idrv: &Opaque<Self::RegType>) {
-        // SAFETY: `idrv` is guaranteed to be a valid `RegType`.
+    unsafe fn unregister(idrv: &Opaque<Self::DriverType>) {
+        // SAFETY: `idrv` is guaranteed to be a valid `DriverType`.
          unsafe { bindings::i2c_del_driver(idrv.get()) }
      }
  }

rust/kernel/pci.rs +11 -7

index 82e128431f08..f58ce35d9c60 100644
@@ -50,13 +50,17 @@
  /// An adapter for the registration of PCI drivers.
  pub struct Adapter<T: Driver>(T);
 
-// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// SAFETY:
+// - `bindings::pci_driver` is a C type declared as `repr(C)`.
+unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
+    type DriverType = bindings::pci_driver;
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` 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;
-
      unsafe fn register(
-        pdrv: &Opaque<Self::RegType>,
+        pdrv: &Opaque<Self::DriverType>,
          name: &'static CStr,
          module: &'static ThisModule,
      ) -> Result {

@@ -68,14 +72,14 @@ unsafe fn register(
              (*pdrv.get()).id_table = T::ID_TABLE.as_ptr();
          }
 
-        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
          to_result(unsafe {
              bindings::__pci_register_driver(pdrv.get(), module.0, name.as_char_ptr())
          })
      }
 
-    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
-        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+    unsafe fn unregister(pdrv: &Opaque<Self::DriverType>) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
          unsafe { bindings::pci_unregister_driver(pdrv.get()) }
      }
  }

rust/kernel/platform.rs +11 -7

index ed889f079cab..e48d055fdc8a 100644
@@ -26,13 +26,17 @@
  /// An adapter for the registration of platform drivers.
  pub struct Adapter<T: Driver>(T);
 
-// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// SAFETY:
+// - `bindings::platform_driver` is a C type declared as `repr(C)`.
+unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
+    type DriverType = bindings::platform_driver;
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` 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;
-
      unsafe fn register(
-        pdrv: &Opaque<Self::RegType>,
+        pdrv: &Opaque<Self::DriverType>,
          name: &'static CStr,
          module: &'static ThisModule,
      ) -> Result {

@@ -55,12 +59,12 @@ unsafe fn register(
              (*pdrv.get()).driver.acpi_match_table = acpi_table;
          }
 
-        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
          to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) })
      }
 
-    unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
-        // SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
+    unsafe fn unregister(pdrv: &Opaque<Self::DriverType>) {
+        // SAFETY: `pdrv` is guaranteed to be a valid `DriverType`.
          unsafe { bindings::platform_driver_unregister(pdrv.get()) };
      }
  }

rust/kernel/usb.rs +9 -5

index d10b65e9fb6a..32f4b2d55dfb 100644
@@ -27,13 +27,17 @@
  /// An adapter for the registration of USB drivers.
  pub struct Adapter<T: Driver>(T);
 
-// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
+// SAFETY:
+// - `bindings::usb_driver` is a C type declared as `repr(C)`.
+unsafe impl<T: Driver + 'static> driver::Driver for Adapter<T> {
+    type DriverType = bindings::usb_driver;
+}
+
+// SAFETY: A call to `unregister` for a given instance of `DriverType` 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::usb_driver;
-
      unsafe fn register(
-        udrv: &Opaque<Self::RegType>,
+        udrv: &Opaque<Self::DriverType>,
          name: &'static CStr,
          module: &'static ThisModule,
      ) -> Result {

@@ -45,14 +49,14 @@ unsafe fn register(
              (*udrv.get()).id_table = T::ID_TABLE.as_ptr();
          }
 
-        // SAFETY: `udrv` is guaranteed to be a valid `RegType`.
+        // SAFETY: `udrv` is guaranteed to be a valid `DriverType`.
          to_result(unsafe {
              bindings::usb_register_driver(udrv.get(), module.0, name.as_char_ptr())

drivers/base/dd.c +3 -0

+	if (dev->driver->p_cb.post_unbind)
+		dev->driver->p_cb.post_unbind(dev);
+#endif
 	arch_teardown_dma_ops(dev);
 	kfree(dev->dma_range_map);
 	dev->dma_range_map = NULL;

include/linux/device/driver.h +2 -0

index cd8e0f0a634b..51a9ebdd8a2d 100644
@@ -85,6 +85,8 @@ enum probe_type {
  *		uevent.
  * @p:		Driver core's private data, no one other than the driver
  *		core can touch this.
+ * @p_cb:	Callbacks private to the driver core; no one other than the
+ *		driver core is allowed to touch this.
  *
  * The device driver-model tracks all of the drivers known to the system.
  * The main reason for this tracking is to enable the driver core to match

include/linux/device/driver.h +4 -1

-impl<T: RegistrationOps> Registration<T> {
+impl<T: RegistrationOps + 'static> Registration<T> {
+    extern "C" fn post_unbind_callback(dev: *mut bindings::device) {
+        // SAFETY: The driver core only ever calls the post unbind callback with a valid pointer to
+        // a `struct device`.

drivers/base/dd.c static DEVICE_ATTR_RW(state_synced); · line 551

Quoted 5 lines · Danilo Krummrich

drivers/base/dd.c +1 -0

@@ -548,6 +548,10 @@ static DEVICE_ATTR_RW(state_synced);
 static void device_unbind_cleanup(struct device *dev)
 {
 	devres_release_all(dev);
+#ifdef CONFIG_RUST

Nit, let's not put #ifdef in .c files, the overhead of an empty pointer for all drivers is not a big deal.

Quoted 3 lines

drivers/base/dd.c +3 -0

+	if (dev->driver->p_cb.post_unbind)
+		dev->driver->p_cb.post_unbind(dev);
+#endif

include/linux/device/driver.h struct device_driver { · line 130

Quoted 6 lines

include/linux/device/driver.h +6 -0

+	struct {
+		/*
+		 * Called after remove() and after all devres entries have been
+		 * processed.
+		 */
+		void (*post_unbind)(struct device *dev);

post_unbind_rust_only()?

Quoted 9 lines

rust/kernel/driver.rs +8 -1

-impl<T: RegistrationOps> Registration<T> {
+impl<T: RegistrationOps + 'static> Registration<T> {
+    extern "C" fn post_unbind_callback(dev: *mut bindings::device) {
+        // SAFETY: The driver core only ever calls the post unbind callback with a valid pointer to
+        // a `struct device`.
+        //
+        // INVARIANT: `dev` is valid for the duration of the `post_unbind_callback()`.
+        let dev = unsafe { &*dev.cast::<device::Device<device::CoreInternal>>() };
+

rust/kernel/driver.rs unsafe impl<T: RegistrationOps> Sync for Registration<T> {} · line 193

Quoted 6 lines

rust/kernel/driver.rs +6 -0

+        // `remove()` and all devres callbacks have been completed at this point, hence drop the
+        // driver's device private data.
+        //
+        // SAFETY: By the safety requirements of the `Driver` trait, `T::DriverData` is the
+        // driver's device private data.
+        drop(unsafe { dev.drvdata_obtain::<T::DriverData>() });

I don't mind this, but why don't we also do this for all C drivers?

Just null out the pointer at this point in time so that no one can touch it, just like you are doing here (in a way.)

Quoted 9 lines · Greg KH

drivers/base/dd.c static DEVICE_ATTR_RW(state_synced); · line 551

Quoted 5 lines · Danilo Krummrich

drivers/base/dd.c +1 -0

@@ -548,6 +548,10 @@ static DEVICE_ATTR_RW(state_synced);
 static void device_unbind_cleanup(struct device *dev)
 {
 	devres_release_all(dev);
+#ifdef CONFIG_RUST

Nit, let's not put #ifdef in .c files, the overhead of an empty pointer for all drivers is not a big deal.

I agree, I mainly did it to make it clear that, as by now, this is only used by Rust driver-core code. However, ...

Quoted 3 lines

Quoted 3 lines

drivers/base/dd.c +3 -0

+	if (dev->driver->p_cb.post_unbind)
+		dev->driver->p_cb.post_unbind(dev);
+#endif

<snip>

Quoted 8 lines

include/linux/device/driver.h struct device_driver { · line 130

Quoted 6 lines

include/linux/device/driver.h +6 -0

+	struct {
+		/*
+		 * Called after remove() and after all devres entries have been
+		 * processed.
+		 */
+		void (*post_unbind)(struct device *dev);

post_unbind_rust_only()?

...this works as well. We can always rename it, in case we start using it in C too.

So, I'm fine with either. :)

Quoted 9 lines

Quoted 9 lines

rust/kernel/driver.rs +8 -1

-impl<T: RegistrationOps> Registration<T> {
+impl<T: RegistrationOps + 'static> Registration<T> {
+    extern "C" fn post_unbind_callback(dev: *mut bindings::device) {
+        // SAFETY: The driver core only ever calls the post unbind callback with a valid pointer to
+        // a `struct device`.
+        //
+        // INVARIANT: `dev` is valid for the duration of the `post_unbind_callback()`.
+        let dev = unsafe { &*dev.cast::<device::Device<device::CoreInternal>>() };
+

Quoted 8 lines

rust/kernel/driver.rs unsafe impl<T: RegistrationOps> Sync for Registration<T> {} · line 193

Quoted 6 lines

rust/kernel/driver.rs +6 -0

+        // `remove()` and all devres callbacks have been completed at this point, hence drop the
+        // driver's device private data.
+        //
+        // SAFETY: By the safety requirements of the `Driver` trait, `T::DriverData` is the
+        // driver's device private data.
+        drop(unsafe { dev.drvdata_obtain::<T::DriverData>() });

I don't mind this, but why don't we also do this for all C drivers?

What exactly do you mean? Manage the lifetime of the device private data commonly in driver-core code?

Quoted 2 lines

Just null out the pointer at this point in time so that no one can touch it, just like you are doing here (in a way.)

I think device_unbind_cleanup() already calls dev_set_drvdata(dev, NULL) [1], so technically we do not have to do it necessarily in Device::drvdata_obtain() as well.

However, with Device::drvdata_obtain() we take back ownership of the Pin<KBox<T>> stored in dev->driver_data, so it makes sense to null out the pointer at exactly this point in time.

I say name it with "rust_" and take out the #ifdef, that makes it simpler/easier to understand.

Quoted 2 lines · Greg KH

I say name it with "rust_" and take out the #ifdef, that makes it simpler/easier to understand.

drivers/base/dd.c +2 -4

index 2d9871503614..bea8da5f8a3a 100644
@@ -548,10 +548,8 @@ static DEVICE_ATTR_RW(state_synced);
 static void device_unbind_cleanup(struct device *dev)
 {
        devres_release_all(dev);
-#ifdef CONFIG_RUST
-       if (dev->driver->p_cb.post_unbind)
-               dev->driver->p_cb.post_unbind(dev);
-#endif
+       if (dev->driver->p_cb.post_unbind_rust)
+               dev->driver->p_cb.post_unbind_rust(dev);
        arch_teardown_dma_ops(dev);
        kfree(dev->dma_range_map);
        dev->dma_range_map = NULL;

include/linux/device/driver.h +2 -4

index 51a9ebdd8a2d..bbc67ec513ed 100644
@@ -121,15 +121,13 @@ struct device_driver {
        void (*coredump) (struct device *dev);
 
        struct driver_private *p;
-#ifdef CONFIG_RUST
        struct {
                /*
                 * Called after remove() and after all devres entries have been
-                * processed.
+                * processed. This is a Rust only callback.
                 */
-               void (*post_unbind)(struct device *dev);
+               void (*post_unbind_rust)(struct device *dev);
        } p_cb;
-#endif
 };
 
 

rust/kernel/driver.rs +0 -0

index 6e32376d4c7c..26095d7bd0d9 100644

Currently, the driver's device private data is allocated and initialized from driver core code called from bus abstractions after the driver's probe() callback returned the corresponding initializer.

Similarly, the driver's device private data is dropped within the remove() callback of bus abstractions after calling the remove() callback of the corresponding driver.

However, commit 6f61a2637abe ("rust: device: introduce Device::drvdata()") introduced an accessor for the driver's device private data for a Device<Bound>, i.e. a device that is currently bound to a driver.

Obviously, this is in conflict with dropping the driver's device private data in remove(), since a device can not be considered to be fully unbound after remove() has finished:

We also have to consider registrations guarded by devres - such as IRQ or class device registrations - which are torn down after remove() in devres_release_all().

Thus, it can happen that, for instance, a class device or IRQ callback still calls Device::drvdata(), which then runs concurrently to remove() (which sets dev->driver_data to NULL and drops the driver's device private data), before devres_release_all() started to tear down the corresponding registration. This is because devres guarded registrations can, as expected, access the corresponding Device<Bound> that defines their scope.

In C it simply is the driver's responsibility to ensure that its device private data is freed after e.g. an IRQ registration is unregistered.

Typically, C drivers achieve this by allocating their device private data with e.g. devm_kzalloc() before doing anything else, i.e. before e.g. registering an IRQ with devm_request_threaded_irq(), relying on the reverse order cleanup of devres [1].

Technically, we could do something similar in Rust. However, the resulting code would be pretty messy:

In Rust we have to differentiate between allocated but uninitialized memory and initialized memory in the type system. Thus, we would need to somehow keep track of whether the driver's device private data object has been initialized (i.e. probe() was successful and returned a valid initializer for this memory) and conditionally call the destructor of the corresponding object when it is freed.

This is because we'd need to allocate and register the memory of the driver's device private data *before* it is initialized by the initializer returned by the driver's probe() callback, because the driver could already register devres guarded registrations within probe() outside of the driver's device private data initializer.

Luckily there is a much simpler solution: Instead of dropping the driver's device private data at the end of remove(), we just drop it after the device has been fully unbound, i.e. after all devres callbacks have been processed.

For this, we introduce a new post_unbind() callback private to the driver-core, i.e. the callback is neither exposed to drivers, nor to bus abstractions.

This way, the driver-core code can simply continue to conditionally allocate the memory for the driver's device private data when the driver's initializer is returned from probe() - no change needed - and drop it when the driver-core code receives the post_unbind() callback.

--

Dependency wise we need a common Driver trait that describes the layout of a specific driver structure, such as struct pci_driver or struct platform_driver. Additional to this specific driver type (which was previously the associated type RegType of the RegistrationOps) it provides the offset to the embedded struct device_driver and the type of the driver's device private data.

This patch series contains two additional dependencies:

(1) A fix for i2c::Driver::shutdown() to not free the driver's device private data at all, which otherwise causes the exact same bug, and is not necessary in the first place anyways.

(2) Add the auxiliary::Driver::unbind() callback. Strictly speaking, this is not a dependency, but without this patch the main fix of this series leaves the remove() callback of the auxiliary bus abstraction with either dead code or quite some code removed; code that we would otherwise add back immediately afterwards.

--

[1] In fact, the cleanup ordering of devres is a separate challenge in Rust, since it is technically unsound to rely on the driver to pick the correct order. I am already working on a solution for this; luckily this also has some synergies with optimizing the required synchronize_rcu() calls required by the Rust Devres container structure down to exactly one per driver unbind.

If a &Device<Bound> lets you access a given value, then we must not destroy that value until after the last &Device<Bound> has expired.

A &Device<Bound> lets you access the driver private data. And a &Device<Bound> lets you access the contents of a Devres<T>.

Thus, the last &Device<Bound> must expire before we destroy driver private data or values inside of Devres<T>. Etc.

Quoted 8 lines · Alice Ryhl

If a &Device<Bound> lets you access a given value, then we must not destroy that value until after the last &Device<Bound> has expired.

A &Device<Bound> lets you access the driver private data. And a &Device<Bound> lets you access the contents of a Devres<T>.

Thus, the last &Device<Bound> must expire before we destroy driver private data or values inside of Devres<T>. Etc.

Yes, the last &Device<Bound> must expire before we destroy the device private data. This is exactly what is achieved by this patch. The device private data is destroyed after all devres callbacks have been processed, which guarantees that there can't be any contexts left that provide a &Device<Bound>.

As for the values inside of a Devres<T>, this is exactly what I refer to in my paragraph above talking about the unsoundness of the devres cleanup ordering in Rust.

I also mention that I'm already working on a solution and it is in fact pretty close to the solution you propose below, i.e. a generic mechanism to support multiple devres domains (which I also see advantages for in C code).

As mentioned, this will also help with getting the required synchronize_rcu() calls down to exactly one per device unbind.

Technically, we could utilize such a devres domain for dropping the device private data, but there is no need to have a separate domain just for this, we already have a distinct place for dropping and freeing the device private data after the device has been fully unbound, which is much simpler than a separate devres domain.

Now, you may argue we don't need a separate devres domain, and that we could use the non-early devres domain. However, this would have the following implication:

In the destructor of the device private data, drivers could still try to use device resources stored in the device private data through try_access(), which may or may not succeed depending on whether the corresponding Devres<T> containers are part of the device private data initializer or whether they have been allocated separately.

Or in other words it would leave room for drivers to abuse this behavior.

Therefore, the desired order is:

1. Driver::unbind() (A place for drivers to tear down the device; registrations are up - unless explicitly revoked by the driver (this is a semantic choice) - and device resources are accessible.)

2. devm_early_* (Drop all devres guarded registrations.)

3. No more &Device<Bound> left.

4. devm_* (Drop all device resources.)

5. No more device resources left.

6. Drop and free device private data. (try_access() will never succeed in the destructor of the device private data.

Quoted 53 lines · Danilo Krummrich

Quoted 8 lines · Alice Ryhl

If a &Device<Bound> lets you access a given value, then we must not destroy that value until after the last &Device<Bound> has expired.

A &Device<Bound> lets you access the driver private data. And a &Device<Bound> lets you access the contents of a Devres<T>.

Thus, the last &Device<Bound> must expire before we destroy driver private data or values inside of Devres<T>. Etc.

Yes, the last &Device<Bound> must expire before we destroy the device private data. This is exactly what is achieved by this patch. The device private data is destroyed after all devres callbacks have been processed, which guarantees that there can't be any contexts left that provide a &Device<Bound>.

As for the values inside of a Devres<T>, this is exactly what I refer to in my paragraph above talking about the unsoundness of the devres cleanup ordering in Rust.

I also mention that I'm already working on a solution and it is in fact pretty close to the solution you propose below, i.e. a generic mechanism to support multiple devres domains (which I also see advantages for in C code).

As mentioned, this will also help with getting the required synchronize_rcu() calls down to exactly one per device unbind.

Technically, we could utilize such a devres domain for dropping the device private data, but there is no need to have a separate domain just for this, we already have a distinct place for dropping and freeing the device private data after the device has been fully unbound, which is much simpler than a separate devres domain.

Now, you may argue we don't need a separate devres domain, and that we could use the non-early devres domain. However, this would have the following implication:

In the destructor of the device private data, drivers could still try to use device resources stored in the device private data through try_access(), which may or may not succeed depending on whether the corresponding Devres<T> containers are part of the device private data initializer or whether they have been allocated separately.

Or in other words it would leave room for drivers to abuse this behavior.

Therefore, the desired order is:

1. Driver::unbind() (A place for drivers to tear down the device; registrations are up - unless explicitly revoked by the driver (this is a semantic choice) - and device resources are accessible.)

2. devm_early_* (Drop all devres guarded registrations.)

3. No more &Device<Bound> left.

Quoted 6 lines

4. devm_* (Drop all device resources.)

5. No more device resources left.

6. Drop and free device private data. (try_access() will never succeed in the destructor of the device private data.

so your private data is just the first devres resource ;)

Ok. I'm worried that when you fix Devres, you have to undo changes you made here. But I guess that's not the end of the world (and maybe you don't have to).

Concept SGTM. I have not yet reviewed patches in details, but

Acked-by Alice Ryhl <[email protected]>

Currently, the driver's device private data is allocated and initialized from driver core code called from bus abstractions after the driver's probe() callback returned the corresponding initializer.

Similarly, the driver's device private data is dropped within the remove() callback of bus abstractions after calling the remove() callback of the corresponding driver.

However, commit 6f61a2637abe ("rust: device: introduce Device::drvdata()") introduced an accessor for the driver's device private data for a Device<Bound>, i.e. a device that is currently bound to a driver.

Obviously, this is in conflict with dropping the driver's device private data in remove(), since a device can not be considered to be fully unbound after remove() has finished:

We also have to consider registrations guarded by devres - such as IRQ or class device registrations - which are torn down after remove() in devres_release_all().

Thus, it can happen that, for instance, a class device or IRQ callback still calls Device::drvdata(), which then runs concurrently to remove() (which sets dev->driver_data to NULL and drops the driver's device private data), before devres_release_all() started to tear down the corresponding registration. This is because devres guarded registrations can, as expected, access the corresponding Device<Bound> that defines their scope.

In C it simply is the driver's responsibility to ensure that its device private data is freed after e.g. an IRQ registration is unregistered.

Typically, C drivers achieve this by allocating their device private data with e.g. devm_kzalloc() before doing anything else, i.e. before e.g. registering an IRQ with devm_request_threaded_irq(), relying on the reverse order cleanup of devres [1].

Technically, we could do something similar in Rust. However, the resulting code would be pretty messy:

In Rust we have to differentiate between allocated but uninitialized memory and initialized memory in the type system. Thus, we would need to somehow keep track of whether the driver's device private data object has been initialized (i.e. probe() was successful and returned a valid initializer for this memory) and conditionally call the destructor of the corresponding object when it is freed.

This is because we'd need to allocate and register the memory of the driver's device private data *before* it is initialized by the initializer returned by the driver's probe() callback, because the driver could already register devres guarded registrations within probe() outside of the driver's device private data initializer.

Luckily there is a much simpler solution: Instead of dropping the driver's device private data at the end of remove(), we just drop it after the device has been fully unbound, i.e. after all devres callbacks have been processed.

For this, we introduce a new post_unbind() callback private to the driver-core, i.e. the callback is neither exposed to drivers, nor to bus abstractions.

This way, the driver-core code can simply continue to conditionally allocate the memory for the driver's device private data when the driver's initializer is returned from probe() - no change needed - and drop it when the driver-core code receives the post_unbind() callback.

--

Dependency wise we need a common Driver trait that describes the layout of a specific driver structure, such as struct pci_driver or struct platform_driver. Additional to this specific driver type (which was previously the associated type RegType of the RegistrationOps) it provides the offset to the embedded struct device_driver and the type of the driver's device private data.

This patch series contains two additional dependencies:

(1) A fix for i2c::Driver::shutdown() to not free the driver's device private data at all, which otherwise causes the exact same bug, and is not necessary in the first place anyways.

(2) Add the auxiliary::Driver::unbind() callback. Strictly speaking, this is not a dependency, but without this patch the main fix of this series leaves the remove() callback of the auxiliary bus abstraction with either dead code or quite some code removed; code that we would otherwise add back immediately afterwards.

--

[1] In fact, the cleanup ordering of devres is a separate challenge in Rust, since it is technically unsound to rely on the driver to pick the correct order. I am already working on a solution for this; luckily this also has some synergies with optimizing the required synchronize_rcu() calls required by the Rust Devres container structure down to exactly one per driver unbind.

Link https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=driver/post_unbind

Danilo Krummrich (6):
rust: i2c: do not drop device private data on shutdown()
rust: auxiliary: add Driver::unbind() callback
rust: driver: introduce a common Driver trait
rust: driver: add DEVICE_DRIVER_OFFSET to the Driver trait
rust: driver: add DriverData type to the generic Driver trait
rust: driver: drop device private data post unbind

  drivers/base/dd.c             |  4 ++
  include/linux/device/driver.h | 11 +++++
  rust/kernel/auxiliary.rs      | 41 +++++++++++++----
  rust/kernel/device.rs         | 20 ++++----
  rust/kernel/driver.rs         | 86 ++++++++++++++++++++++++++++-------
  rust/kernel/i2c.rs            | 31 ++++++++-----
  rust/kernel/pci.rs            | 27 +++++++----
  rust/kernel/platform.rs       | 27 +++++++----
  rust/kernel/usb.rs            | 27 +++++++----
  9 files changed, 203 insertions(+), 71 deletions(-)

base-commit: 8510ef5e3cfbd7d59a16845f85cd0194a8689761

Danilo Krummrich (6):
rust: i2c: do not drop device private data on shutdown()
rust: auxiliary: add Driver::unbind() callback
rust: driver: introduce a common Driver trait

rust: driver: add DEVICE_DRIVER_OFFSET to the Driver trait
rust: driver: add DriverData type to the generic Driver trait
rust: driver: drop device private data post unbind