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