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

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

[Sami Tolvanen]

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.

GpuVaAlloc wraps only uninitialised memory and exposes none of it, so it
is unconditionally Send and Sync. GpuVmBo is an atomically refcounted
handle whose accessors hand out the driver data and GEM object by shared
reference and whose deferred put drops them, so its Send and Sync impls
are bounded on T::VmBoData and T::Object.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
Changes in v2:
- Added a missing T::Object: Send + Sync bound pointed out by Sashiko.

---
 rust/kernel/drm/gpuvm/va.rs    |  8 ++++++++
 rust/kernel/drm/gpuvm/vm_bo.rs | 21 +++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/rust/kernel/drm/gpuvm/va.rs b/rust/kernel/drm/gpuvm/va.rs
index 0b09fe44ab39..dcb2dec4fbdf 100644
--- a/rust/kernel/drm/gpuvm/va.rs
+++ b/rust/kernel/drm/gpuvm/va.rs
@@ -104,6 +104,14 @@ pub fn vm_bo(&self) -> &GpuVmBo<T> {
 /// The memory is zeroed.
 pub struct GpuVaAlloc<T: DriverGpuVm>(KBox<MaybeUninit<GpuVa<T>>>);
 
+// SAFETY: A [`GpuVaAlloc`] is an owned, uninitialised allocation with no live `T::VaData` and no
+// thread-bound state.
+unsafe impl<T: DriverGpuVm> Send for GpuVaAlloc<T> {}
+
+// SAFETY: A [`GpuVaAlloc`] has no `&self` method that reaches its contents, so a shared
+// `&GpuVaAlloc` cannot access the allocation.
+unsafe impl<T: DriverGpuVm> Sync for GpuVaAlloc<T> {}
+
 impl<T: DriverGpuVm> GpuVaAlloc<T> {
     /// Pre-allocate a [`GpuVa`] object.
     pub fn new(flags: AllocFlags) -> Result<GpuVaAlloc<T>, AllocError> {
diff --git a/rust/kernel/drm/gpuvm/vm_bo.rs b/rust/kernel/drm/gpuvm/vm_bo.rs
index c064ac63897b..fa183f6306ff 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,
+{
+}
+
+// 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,
+{
+}
+
 // SAFETY: By type invariants, the allocation is managed by the refcount in `self.inner`.
 unsafe impl<T: DriverGpuVm> AlwaysRefCounted for GpuVmBo<T> {
     fn inc_ref(&self) {

base-commit: fea3a2dd7d3fc1936211ced5f84420e610435730
-- 
2.54.0.1032.g2f8565e1d1-goog

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

[sashiko-bot]

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