[PATCH] rust: i2c: avoid locking when calling I2cAdapter::inc_ref

The current implementation of `<I2cAdapter as
AlwaysRefCounted>::inc_ref` relies on the C function `i2c_get_adapter`
to increment module and device counters. This function acquires a lock,
looks for the adapter in the IDR table, and, if found, increments the
counters before returning the adapter.

In the Rust API, the `I2cAdapter::get` method returns an
`ARef<I2cAdapter>` upon success. Incrementing this reference count in an
atomic context (for example, via `ARef::clone`, which relies on
`AlwaysRefCounted::inc_ref`) could trigger a sleep-in-atomic bug due to the
mutex locking inside `i2c_get_adapter`.

Since cloning an `ARef` implies we already hold a valid reference to
the adapter, the IDR table lookup and its associated lock are
unnecessary. The fix consists of bypassing `i2c_get_adapter` and
instead calling `__module_get` and `get_device` directly to increment
the counters.

Reported-by: Sashiko <[email protected]>
Closes: https://sashiko.dev/#/patchset/[email protected]
Signed-off-by: Nicolás Antinori <[email protected]>
---
Citing the second part of Sashiko's report:

> Furthermore, if the adapter is unregistered and removed from the IDR,
> bindings::i2c_get_adapter() will return NULL and fail to increment the
> reference count. Since inc_ref() ignores the return value, wouldn't dropping
> that cloned ARef unconditionally call dec_ref() (i2c_put_adapter)?

`inc_ref` no longer relies on `i2c_get_adapter` to increment the
reference counts for the module and the device. Also, being able to
execute `<I2cAdapter as AlwaysRefCounted>::inc_ref` implies the
existence of a shared reference, meaning that the adapter is still
registered in the IDR.

> Could this lead to an underflow, double-put, and a use-after-free of the
> adapter and its module? Or if the IDR index was reused, might it increment
> the new adapter's refcount while decrementing the old one twice?

I don't believe this situation is possible.

When `i2c_del_adapter` is executed in `i2c-core-base.c`, the kernel waits for
all references to be dropped prior to removing the device from the IDR.

This guarantees that no `ARef` is still alive when the IDR removal happens,
effectively eliminating the risk of an underflow, double-put, or calling
`dec_ref` on an invalid reference.

 rust/kernel/i2c.rs | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
index 624b971ca8b0..d89c42691dfe 100644
--- a/rust/kernel/i2c.rs
+++ b/rust/kernel/i2c.rs
@@ -426,8 +426,11 @@ pub fn get(index: i32) -> Result<ARef<Self>> {
 // SAFETY: Instances of `I2cAdapter` are always reference-counted.
 unsafe impl AlwaysRefCounted for I2cAdapter {
     fn inc_ref(&self) {
-        // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
-        unsafe { bindings::i2c_get_adapter(self.index()) };
+        // SAFETY: The existence of a shared reference guarantees that the refcounts are non-zero.
+        unsafe {
+            bindings::__module_get((*self.as_raw()).owner);
+            bindings::get_device(&raw mut (*self.as_raw()).dev);
+        }
     }

     unsafe fn dec_ref(obj: NonNull<Self>) {
--
2.47.3