Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] `DmaResvGuard` implicitly implements `Send`, allowing the `ww_mutex` to be unlocked on a different thread than the one that acquired it, violating strict kernel mutex semantics. - [Medium] Missing `#[must_use]` attribute on the `DmaResvGuard` lock guard. - [Low] Incorrect use of `#[inline(always)]` for small abstraction functions that do not use `build_assert!()`. --
commit 83465c025b759f686444735d2b770912987eb833 Author: Lyude Paul <[email protected]> rust: drm: gem: shmem: Add DmaResvGuard helper Just a temporary holdover to make locking/unlocking the dma_resv lock much easier. > diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs > index 34af402899a0e..4cf038f7afb71 100644 > --- a/rust/kernel/drm/gem/shmem.rs > +++ b/rust/kernel/drm/gem/shmem.rs [ ... ] > @@ -235,3 +238,29 @@ impl<T: DriverObject, C: DeviceContext> > driver::AllocImpl for Object<T, C> { > dumb_map_offset: None, > }; > } > + > +/// Private helper-type for holding the `dma_resv` object for a GEM shmem > object. > +/// > +/// When this is dropped, the `dma_resv` lock is dropped as well. > +/// > +// TODO: This should be replace with a WwMutex equivalent once we have such > bindings in the kernel. > +struct DmaResvGuard<'a, T: DriverObject, C: DeviceContext = Registered>(&'a > Object<T, C>); [Severity: High] Does this struct implicitly implement Send? Since the wrapped type Object<T, C> is Sync, a shared reference &'a Object<T, C> is both Send and Sync. This makes DmaResvGuard implicitly Send. However, bindings::dma_resv_lock acquires a ww_mutex, and its Drop implementation releases it. Linux kernel mutexes are strictly bound to the task (current) that acquired them. If safe Rust code sends the DmaResvGuard to another thread and drops it there, mutex_unlock will be called by a non-owner thread. Could this cause kernel panics or break mutual exclusion, and should DmaResvGuard be explicitly marked as !Send? [Severity: Medium] Should this struct be marked with #[must_use]? Without #[must_use], users might inadvertently call DmaResvGuard::new(obj) as a standalone statement without binding it to a variable, causing the guard to drop immediately. This would leave the subsequent critical section unprotected. > + > +impl<'a, T: DriverObject, C: DeviceContext> DmaResvGuard<'a, T, C> { > + #[inline(always)] [Severity: Low] This isn't a bug, but does this violate the Rust subsystem coding guidelines? According to Documentation/rust/coding-guidelines.rst, for abstractions only: functions that are small or forwarding to a binding call should be annotated with #[inline]. The #[inline(always)] annotation is strictly reserved for functions using build_assert!() that depend on function parameters. > + #[expect(unused)] > + fn new(obj: &'a Object<T, C>) -> Self { > + // SAFETY: This lock is initialized throughout the lifetime of > `object`. > + unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), > ptr::null_mut()) }; > + > + Self(obj) > + } > +} > + > +impl<'a, T: DriverObject, C: DeviceContext> Drop for DmaResvGuard<'a, T, C> { > + #[inline(always)] [Severity: Low] This isn't a bug, but should this be #[inline] instead of #[inline(always)]? As mentioned above, the guidelines reserve #[inline(always)] for functions using build_assert!() that depend on function parameters. > + fn drop(&mut self) { > + // SAFETY: We are releasing the lock grabbed during the creation of > this object. > + unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) }; > + } > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
