Re: [PATCH v2] rust: drm: gpuvm: implement Send and Sync for GpuVaAlloc and GpuVmBo

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing `Send` bounds in the newly added `Sync` implementation for `GpuVmBo<T>`, allowing `!Send` types to be dropped across threads.

Pre-existing issues:
- [High] Thread-safety violation due to C-level deferred cleanup queue bypassing conditional `Send` bounds.
--

commit 34b8a98e3e88f5d7b03fbb66084e780d4c56de5f
Author: Sami Tolvanen <samitolvanen@google.com>

rust: drm: gpuvm: implement Send and Sync for GpuVaAlloc and GpuVmBo

Moving a GpuVaAlloc or GpuVmBo between threads currently forces drivers
to write their own unsafe Send and Sync impls. Provide the markers in
the abstraction instead.

> diff --git a/rust/kernel/drm/gpuvm/vm_bo.rs b/rust/kernel/drm/gpuvm/vm_bo.rs
> index c064ac63897be..fa183f6306ff8 100644
> --- a/rust/kernel/drm/gpuvm/vm_bo.rs
> +++ b/rust/kernel/drm/gpuvm/vm_bo.rs

[ ... ]

> @@ -19,6 +19,27 @@ pub struct GpuVmBo<T: DriverGpuVm> {
>      data: T::VmBoData,
>  }
>  
> +// SAFETY: The refcount in `self.inner` is atomic, so `dec_ref`'s deferred put is sound from any
> +// thread. `drm_gpuvm_bo_deferred_cleanup` drops `data` and the last GEM-object reference on
> +// whichever thread drains the queue, hence the `T::VmBoData: Send` and `T::Object: Send` bounds.
> +// [`Self::obj`] hands out `&T::Object` for a shared object, so the move needs `T::Object: Sync`.
> +unsafe impl<T: DriverGpuVm> Send for GpuVmBo<T>
> +where
> +    T::VmBoData: Send,
> +    T::Object: Send + Sync,
> +{
> +}

[Severity: High]
This is a pre-existing issue, but does the C-level deferred cleanup queue
bypass these conditional Send bounds?

Since the bo_defer queue acts as a hidden cross-thread channel, when a local
ARef<GpuVmBo<T>> is dropped by its creating thread, it is unconditionally
placed on the GpuVm's bo_defer queue via dec_ref.

Because GpuVm<T> is universally Send + Sync, a second thread can safely call
gpuvm.deferred_cleanup(). This pops the BO and executes its destructor on the
second thread, bypassing the conditional Send bounds on GpuVmBo, because the
BO is never explicitly moved via Rust semantics.

Would it be safer to enforce Send bounds unconditionally on the VmBoData and
VaData associated types within the DriverGpuVm trait itself?

> +
> +// SAFETY: The fields of `inner` read by shared-reference methods are immutable after construction.
> +// [`Self::data`] hands out `&T::VmBoData` and [`Self::obj`] hands out `&T::Object`, so sharing
> +// `&Self` across threads requires both to be `Sync`.
> +unsafe impl<T: DriverGpuVm> Sync for GpuVmBo<T>
> +where
> +    T::VmBoData: Sync,
> +    T::Object: Sync,
> +{
> +}

[Severity: High]
Since GpuVmBo<T> implements the AlwaysRefCounted trait, can a thread with a
shared reference &GpuVmBo<T> upgrade it to an owned reference ARef<GpuVmBo<T>>
via ARef::from(&*bo)?

If a thread does this and becomes the last reference holder, dropping the ARef
will execute the GpuVmBo<T> destructor, dropping T::VmBoData and T::Object on
that thread.

Similar to Arc<T>, should Sync for GpuVmBo<T> also require the dropped types
to be Send (i.e. T::VmBoData: Send and T::Object: Send) to prevent a safe Rust
program from dropping a !Send type on another thread?

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