Re: [PATCH v7] rust: aref: make `AlwaysRefCounted::inc_ref` an associated function

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] `I2cAdapter::inc_ref` calls `i2c_get_adapter()` which sleeps and can fail, leading to sleep-in-atomic panics and Use-After-Free.
--

commit dcdb7e5879ef4f6a18646d7f509abd59ae6f10db
Author: Trevor Chan <trev@trevrosa.dev>

rust: aref: make AlwaysRefCounted::inc_ref an associated function

This commit changes the AlwaysRefCounted::inc_ref trait method to be an
associated function rather than taking a self reference. This prevents
accidental calls to inc_ref on types that implement it, and updates all
implementors across various kernel subsystems to match the new signature.

Link: https://github.com/Rust-for-Linux/linux/issues/1177

[ ... ]

> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> index 624b971ca8b0b..1a9882a64c4b8 100644
> --- a/rust/kernel/i2c.rs
> +++ b/rust/kernel/i2c.rs
> @@ -425,9 +425,9 @@ 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) {
> +    fn inc_ref(obj: &Self) {
>          // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero.
> -        unsafe { bindings::i2c_get_adapter(self.index()) };
> +        unsafe { bindings::i2c_get_adapter(obj.index()) };

[Severity: High]
This isn't a bug introduced by this patch, but is it safe for
I2cAdapter::inc_ref to call i2c_get_adapter here?

Since i2c_get_adapter acquires a mutex (core_lock), won't cloning an
ARef<I2cAdapter> in an atomic context (such as under a spinlock) trigger
a sleep-in-atomic panic?

Additionally, does this code handle the case where i2c_get_adapter fails?
If the adapter has been unregistered (e.g., the device is hot-unplugged or
the driver unbound) while the ARef is still held, the IDR lookup fails and
i2c_get_adapter returns NULL.

Because this return value is ignored in inc_ref, the underlying device
and module refcounts wouldn't actually be incremented. When the cloned ARef
is subsequently dropped, dec_ref will unconditionally decrement the
refcounts, which could underflow and cause a use-after-free while other
ARef instances still point to it.

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

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260628100731.64885-1-trev@trevrosa.dev?part=1