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 <[email protected]> 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/[email protected]?part=1
