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

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

[Nicolás Antinori]

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 <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260524181151.24988-1-nico.antinori.7@gmail.com
Signed-off-by: Nicolás Antinori <nico.antinori.7@gmail.com>
---
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

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

[Nicolás Antinori]

Hello,

Please do not apply this patch.

On Mon Jun 15, 2026 at 5:10 PM -03, Nicolás Antinori wrote:
> 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 <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260524181151.24988-1-nico.antinori.7@gmail.com
> Signed-off-by: Nicolás Antinori <nico.antinori.7@gmail.com>
> ---
> 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);

I got a reply for this patch from Sashiko pointing out that the bindings
for `__module_get` won't generate if `CONFIG_MODULES=n` or
`CONFIG_MODULE_UNLOAD=n` because the function is defined as static
inline in those cases, which causes the build to fail. The diagnostic is
correct.

I sincerely apologize for this oversight. I'll work on a v2 patch to fix
this (I believe conditional compilation based on the config should be
sufficient).

Here's the review link:
https://sashiko.dev/#/patchset/20260615201141.8920-1-nico.antinori.7@gmail.com

Regards

> +            bindings::get_device(&raw mut (*self.as_raw()).dev);
> +        }
>      }
>
>      unsafe fn dec_ref(obj: NonNull<Self>) {
> --
> 2.47.3

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

[Gary Guo]

On Mon Jun 15, 2026 at 9:10 PM BST, Nicolás Antinori wrote:
> 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);

Instead of open coding this sequence, it would be better to add a C API that
does exactly this (getting another reference from existing one).

That said, is there an actual user that needs this, or are we just implementing
AlwaysRefCounted preemptively?

Best,
Gary

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

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

[Nicolás Antinori]

Hello Gary,

On Wed Jun 17, 2026 at 11:12 AM -03, Gary Guo wrote:
> On Mon Jun 15, 2026 at 9:10 PM BST, Nicolás Antinori wrote:
>> 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);
>
> Instead of open coding this sequence, it would be better to add a C API that
> does exactly this (getting another reference from existing one).

I based this solution on the logic used in I2cClient (where only
get_device is needed) and by verifying which counters the C
implementation (i2c_get_adapter) increments.

The i2c_get_adapter function in i2c-core-base.c performs two increments
(module increment is done by calling try_module_get, but in this case,
because inc_ref operates on an already live instance, unconditionally 
incrementing the count with `__module_get` should be safe).

If I understand correctly, the idea would be to introduce a helper 
function on the C side, for example:
void i2c_adapter_increment(struct i2c_adap *adap);

We would perform the increments there and call it from Rust. Using this 
in inc_ref would be safe because the existence of &self guarantees that 
we already have a valid, live instance.

Is there any concern regarding this function beign exposed to the C
side? To use it safely in C, callers would have to ensure that *adap
points to a valid instance.

>
> That said, is there an actual user that needs this, or are we just implementing
> AlwaysRefCounted preemptively?

The only user I could find at the moment is the example driver
(impl platform::Driver for SampleDriver ..) in
samples/rust/rust_i2c_client.rs.

Implementing AlwaysRefCounted is required for types wrapped in ARef,
which is an smart pointer that handles custom reference counting.
Since I2cAdapter::get returns an ARef<I2cAdapter> upon success, the 
trait must be implemented.

Thank you!

>
> Best,
> Gary
>
>> +        }
>>      }
>>
>>      unsafe fn dec_ref(obj: NonNull<Self>) {
>> --
>> 2.47.3