Re: [PATCH v18 1/4] rust: drm: gem: shmem: Add DmaResvGuard helper

[sashiko-bot@kernel.org]

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 <lyude@redhat.com>

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/20260604192740.659240-1-lyude@redhat.com?part=1