Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] VMap unconditionally implements Send and Sync without requiring its generic parameter R to be Send or Sync. - [High] The Sync implementation for VMap combined with its IoCapable trait implementations allows safe concurrent volatile writes to the same memory location, causing a data race. - [Medium] Off-by-one error in vmap_io kunit test loop prematurely terminates the test, preventing verification of the final byte. - [Low] Struct VMap fails to compile due to unused generic type parameters D and C. --
commit 2c1f27a93b4873be7178ca4b1861ef62ac59a6b1 Author: Lyude Paul <[email protected]> rust: drm: gem: shmem: Add vmap functions This commit hooks up vmap creation for gem shmem objects in the Rust bindings, allowing mappings to be created for their contents. > diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs > index bfbf2bb25eac1..d4afc06d1be50 100644 > --- a/rust/kernel/drm/gem/shmem.rs > +++ b/rust/kernel/drm/gem/shmem.rs [ ... ] > +/// A reference to a virtual mapping for an shmem-based GEM object in kernel > address space. > +/// > +/// # Invariants > +/// > +/// - The size of `owner` is >= SIZE. > +/// - The memory pointed to by `addr` remains valid at least until this > object is dropped. > +pub struct VMap<D, R, C = Registered, const SIZE: usize = 0> > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref<Target = Object<D, C>>, > +{ > + addr: *mut c_void, > + owner: R, > +} [Severity: Low] Does this struct compile successfully? The generic type parameters D and C are used in the where clause but not in any fields. Would this trigger a compiler error (E0392) requiring a PhantomData field to compute variance? [ ... ] > +// SAFETY: VMaps are safe to send across threads. > +unsafe impl<D, R, C, const SIZE: usize> Send for VMap<D, R, C, SIZE> > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref<Target = Object<D, C>>, > +{ > +} > + > +// SAFETY: VMaps are safe to access from multiple threads. > +unsafe impl<D, R, C, const SIZE: usize> Sync for VMap<D, R, C, SIZE> > +where > + D: DriverObject, > + C: DeviceContext, > + R: Deref<Target = Object<D, C>>, > +{ > +} [Severity: High] Are these unconditional Send and Sync implementations sound? If a caller provides a custom type for R that implements Deref but is !Send or !Sync (such as a type using non-atomic reference counting), VMap would still be allowed across threads. When the VMap is dropped on another thread, the !Send R would be dropped there, resulting in a data race. Should these implementations require explicit Send and Sync bounds on R? [ ... ] > +macro_rules! impl_vmap_io_capable { > + ($ty:ty) => { > + impl<D, R, C, const SIZE: usize> IoCapable<$ty> for VMap<D, R, C, > SIZE> > + where > + D: DriverObject, > + C: DeviceContext, > + R: Deref<Target = Object<D, C>>, > + { > + #[inline] > + unsafe fn io_read(&self, address: usize) -> $ty { > + let ptr = address as *mut $ty; > + > + // SAFETY: The safety contract of `io_read` guarantees that > address is a valid > + // address within the bounds of `Self` of at least the size > of $ty, and is properly > + // aligned. > + unsafe { ptr::read_volatile(ptr) } > + } > + > + #[inline] > + unsafe fn io_write(&self, value: $ty, address: usize) { > + let ptr = address as *mut $ty; > + > + // SAFETY: The safety contract of `io_write` guarantees that > address is a valid > + // address within the bounds of `Self` of at least the size > of $ty, and is properly > + // aligned. > + unsafe { ptr::write_volatile(ptr, value) } > + } > + } > + }; > +} [Severity: High] Can this result in a data race? Since VMap implements the Sync trait, shared references (&VMap) can be safely sent to multiple threads. This IoCapable implementation powers safe Io trait accessors (like write8) using ptr::write_volatile, which is not atomic. According to the Rust memory model, concurrent volatile writes from multiple threads to the same memory location constitute a data race. Should this synchronize access to avoid undefined behavior? [ ... ] > + #[test] > + fn vmap_io() -> Result { > + let (_dev, drm) = create_drm_dev()?; > + > + let obj = Object::<KunitObject, _>::new(&drm, PAGE_SIZE, > ObjectConfig::default(), ())?; > + > + let vmap = obj.vmap::<PAGE_SIZE>()?; > + > + vmap.write8(0xDE, 0x0); > + assert_eq!(vmap.read8(0x0), 0xDE); > + vmap.write32(0xFEDCBA98, 0x20); > + > + assert_eq!(vmap.read32(0x20), 0xFEDCBA98); > + > + // Ensure the ordering in memory is correct > + let offsets = (0x20..0x23).into_iter(); > + let expected = 0xFEDCBA98_u32.to_ne_bytes().into_iter(); > + for (offset, expected) in offsets.zip(expected) { > + assert_eq!(vmap.read8(offset), expected); > + } > + > + Ok(()) > + } > +} [Severity: Medium] Does this loop skip the 4th byte verification? The offsets iterator (0x20..0x23) produces exactly 3 elements (0x20, 0x21, and 0x22). Because zip stops when the shortest iterator is exhausted, it appears the 4th byte at offset 0x23 is silently skipped and never verified. Should this be (0x20..=0x23) or (0x20..0x24) instead? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
