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

["Nicolás Antinori" <nico.antinori.7@gmail.com>]

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