Re: [PATCH v3 2/2] rust: drm: gpuvm: require Send and Sync for GpuVm's shared data

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] GpuVm's Send and Sync implementations omit bounds for T::VaData, allowing !Send data to be moved across threads, leading to data races.
--

commit 2dbb42abd1672fd0f3c87ba84b8650b37d9c9e84
Author: Sami Tolvanen <samitolvanen@google.com>

rust: drm: gpuvm: require Send and Sync for GpuVm's shared data

This commit adds explicit Send and Sync bounds for T::VmBoData and
T::Object to the GpuVm Send and Sync implementations. It addresses
unsoundness when obtain() aliases these types across threads.

> diff --git a/rust/kernel/drm/gpuvm/mod.rs b/rust/kernel/drm/gpuvm/mod.rs
> index ae58f6f667c1f..398068f2eb4f7 100644
> --- a/rust/kernel/drm/gpuvm/mod.rs
> +++ b/rust/kernel/drm/gpuvm/mod.rs
> @@ -74,9 +74,25 @@ pub struct GpuVm<T: DriverGpuVm> {
>  
>  // SAFETY: The GPUVM api does not assume that it is tied to a specific thread. The destructor will
>  // drop the `data` field, which is okay because it is guaranteed `Send` by the `DriverGpuVm` trait.
> -unsafe impl<T: DriverGpuVm> Send for GpuVm<T> {}
> -// SAFETY: The GPUVM api is designed to allow &self methods to be called in parallel.
> -unsafe impl<T: DriverGpuVm> Sync for GpuVm<T> {}
> +// `obtain()` called from several threads can return `ARef`s to the same bo, aliasing `T::VmBoData`
> +// and `T::Object`, and `deferred_cleanup()` can drop them from any thread, so both must be
> +// `Send + Sync`.
> +unsafe impl<T: DriverGpuVm> Send for GpuVm<T>
> +where
> +    T::VmBoData: Send + Sync,
> +    T::Object: Send + Sync,
> +{
> +}

[Severity: Critical]
This is a pre-existing issue, but does GpuVm also need Send + Sync bounds
for T::VaData here?

T::VaData is stored within GpuVa<T>, which is logically owned by the GPUVM's
interval tree. Because GpuVm<T> does not require T::VaData to be Send + Sync,
a user could implement DriverGpuVm with a !Send type for VaData.

Since GpuVm<T> explicitly bypasses the lack of Send on VaData,
UniqueRefGpuVm<T> automatically derives Send. This allows safe code to map
a VA containing !Send data on one thread, send the UniqueRefGpuVm<T> to
another thread, and unmap the VA there.

Unmapping via sm_unmap() returns a GpuVaRemoved<T> which drops the !Send
VaData on the wrong thread, potentially causing data races.

> +// SAFETY: The GPUVM api is designed to allow &self methods to be called in parallel. `obtain()`
> +// called from several threads can return `ARef`s to the same bo, aliasing `T::VmBoData` and
> +// `T::Object`, and `deferred_cleanup()` can drop them from any thread, so both must be
> +// `Send + Sync`.
> +unsafe impl<T: DriverGpuVm> Sync for GpuVm<T>
> +where
> +    T::VmBoData: Send + Sync,
> +    T::Object: Send + Sync,
> +{
> +}

[Severity: Critical]
This is a pre-existing issue, but should the Sync implementation also include
Send + Sync bounds for T::VaData for the same reasons?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609003256.1829625-4-samitolvanen@google.com?part=2