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 <[email protected]> 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/[email protected]?part=1
