Re: [PATCH 5/7] rust: i2c: remove the unbind() callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] rust: i2c: dropping Driver::Data in post_unbind_rust runs after
  devres release
- [Medium] rust: i2c: missing Sync impl for I2cClient<device::Bound>
--

commit 5a5ad706d767f7788672a5c38d2627a235e83cd2
Author: Danilo Krummrich <dakr@kernel.org>

rust: i2c: remove the unbind() callback

This commit removes i2c::Driver::unbind() and the adapter's remove
callback, documenting that I2C driver teardown should be performed in
Drop or PinnedDrop of Driver::Data.

> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> index b4ff25c830fc0..b15e73bf827ab 100644
> --- a/rust/kernel/i2c.rs
> +++ b/rust/kernel/i2c.rs
> @@ -123,7 +123,6 @@ unsafe fn register(
>          unsafe {
>              (*idrv.get()).driver.name = name.as_char_ptr();
>              (*idrv.get()).probe = Some(Self::probe_callback);
> -            (*idrv.get()).remove = Some(Self::remove_callback);
>              (*idrv.get()).shutdown = Some(Self::shutdown_callback);
>              (*idrv.get()).id_table = i2c_table;
>              (*idrv.get()).driver.of_match_table = of_table;
> @@ -161,18 +160,6 @@ extern "C" fn probe_callback(idev: *mut bindings::i2c_client) -> kernel::ffi::c_
>          })
>      }
>  
> -    extern "C" fn remove_callback(idev: *mut bindings::i2c_client) {
> -        // SAFETY: `idev` is a valid pointer to a `struct i2c_client`.
> -        let idev = unsafe { &*idev.cast::<I2cClient<device::CoreInternal<'_>>>() };
> -
> -        // SAFETY: `remove_callback` is only ever called after a successful call to
> -        // `probe_callback`, hence it's guaranteed that `I2cClient::set_drvdata()` has been called
> -        // and stored a `Pin<KBox<T::Data<'_>>>`.
> -        let data = unsafe { idev.as_ref().drvdata_borrow::<T::Data<'_>>() };
> -
> -        T::unbind(idev, data);
> -    }
> -
>      extern "C" fn shutdown_callback(idev: *mut bindings::i2c_client) {
>          // SAFETY: `shutdown_callback` is only ever called for a valid `idev`
>          let idev = unsafe { &*idev.cast::<I2cClient<device::CoreInternal<'_>>>() };

[Severity: High]
Does moving I2C driver teardown from remove_callback() to the Drop or
PinnedDrop destructor of Driver::Data cause driver teardown to run after
the client's devres resources have already been released?

i2c_device_probe() opens an unclosed devres group on client->dev before
calling driver->probe(), so devres entries registered for the client land
in client->devres_group_id.  On unbind, __device_release_driver() calls
device_remove() -> i2c_device_remove() before device_unbind_cleanup() ->
post_unbind_rust():

drivers/i2c/i2c-core-base.c:i2c_device_remove() {
	...
	if (driver->remove) {
		...
		driver->remove(client);
	}

	debugfs_remove_recursive(client->debugfs);

	devres_release_group(&client->dev, client->devres_group_id);
	...
}

Because post_unbind_rust() drops Driver::Data in device_unbind_cleanup()
after i2c_device_remove() has already released client->devres_group_id,
all devres resources on the client are already revoked and dropped before
Driver::Data::drop() runs.  If Driver::Data retains a &'bound reference
from Devres::access() or calls Devres::access() with a retained &'bound
Device<Bound> in its destructor, Devres::access() skips the Revocable
availability check and causes a use-after-free in safe Rust.

> @@ -312,6 +299,10 @@ pub trait Driver {
>      type IdInfo: 'static;
>  
>      /// The type of the driver's bus device private data.
> +    ///
> +    /// Device teardown should be performed in [`Drop`] or [`PinnedDrop`](pin_init::PinnedDrop)
> +    /// for this type. The `'bound` lifetime allows the data to retain device resources and a
> +    /// [`I2cClient<device::Bound>`] reference for use during teardown.
>      type Data<'bound>: Send + 'bound;

[Severity: Medium]
Can a driver actually store &'bound I2cClient<device::Bound> in
Driver::Data<'bound> as documented here?

Driver::Data<'bound> requires Send, so &'bound I2cClient<device::Bound>
is only Send if I2cClient<device::Bound> implements Sync.  However, Sync
is only implemented for the default context parameter I2cClient
(I2cClient<device::Normal>), not for I2cClient<Ctx> or
I2cClient<device::Bound>, so storing &'bound I2cClient<device::Bound> in
Driver::Data<'bound> fails to compile.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112005455.1224352-1-dakr@kernel.org?part=5