Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] The `IoCapable` implementation for `VMap` uses non-volatile `ptr::read` and `ptr::write`, allowing the compiler to optimize away or reorder memory accesses to shared device memory. - [Medium] `VMap` and `VMapOwned` are implicitly `!Send` and `!Sync` due to the `*mut c_void` field, artificially restricting the mapping to the thread that created it. - [Low] Missing `// INVARIANT:` comment explaining `VMap` memory validity at its construction site, violating subsystem guidelines. --
commit b418d73acbe560c4eda85331900333d19c6ae108 Author: Lyude Paul <[email protected]> rust: drm: gem: shmem: Add vmap functions One of the more obvious use cases for gem shmem objects is the ability to create mappings into their contents. So, let's hook this up in our rust bindings. > diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs > index 4cf038f7afb71..a20b0c0dac6bd 100644 > --- a/rust/kernel/drm/gem/shmem.rs > +++ b/rust/kernel/drm/gem/shmem.rs [ ... ] > @@ -188,6 +196,80 @@ extern "C" fn free_callback(obj: *mut > bindings::drm_gem_object) { > // SAFETY: We're recovering the Kbox<> we created in > gem_create_object() > let _ = unsafe { KBox::from_raw(this) }; > } > + > + /// Attempt to create a vmap from the gem object, and confirm the size > of said vmap. > + fn make_vmap<'a, R, const SIZE: usize>(&'a self) -> Result<VMap<T, R, C, > SIZE>> > + where > + R: Deref<Target = Self> + From<&'a Self>, > + { [ ... ] > + // XXX: We don't currently support iomem allocations > + if map.is_iomem { > + // SAFETY: > + // - The vmap operation above succeeded, guaranteeing that `map` > points to a valid > + // memory mapping. > + // - We checked that this is an iomem allocation, making it safe > to read vaddr_iomem > + unsafe { self.raw_vunmap(map) }; > + > + Err(ENOTSUPP) > + } else { > + Ok(VMap { > + // SAFETY: We checked that this is not an iomem allocation, > making it safe to read > + // vaddr > + addr: unsafe { map.__bindgen_anon_1.vaddr }, > + owner: self.into(), > + }) [Severity: Low] This isn't a bug, but there's a missing invariant comment here. The Rust subsystem guidelines mention that when a struct with an # Invariants documentation section is constructed, the code should have an // INVARIANT: comment explaining why the invariants are satisfied. Would it make sense to add a comment here explaining why the memory remains valid until dropped? > + } > + } [ ... ] > @@ -264,3 +345,232 @@ fn drop(&mut self) { > unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) }; > } > } > + > +macro_rules! impl_vmap_io_capable { > + ($impl:ident, $ty:ty) => { > + impl<D, R, C, const SIZE: usize> IoCapable<$ty> for $impl<D, R, C, > SIZE> > + where > + D: DriverObject, > + C: DeviceContext, > + R: Deref<Target = Object<D, C>>, > + { > + #[inline(always)] > + 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(ptr) } > + } [Severity: High] Can this allow the compiler to improperly optimize these memory accesses? Since VMap represents GEM shmem objects mapped into the kernel virtual address space, this memory might be concurrently accessed by hardware or userspace. Using standard non-volatile ptr::read and ptr::write allows the compiler to cache values in registers, coalesce writes, or elide them entirely. The C equivalent (iosys_map_rd and iosys_map_wr) uses READ_ONCE() and WRITE_ONCE() for this reason. Should this use ptr::read_volatile() to safely access the shared memory? > + > + #[inline(always)] > + 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(ptr, value) } > + } [Severity: High] Similar to the io_read() implementation above, does this write need to use ptr::write_volatile() to prevent the compiler from caching or coalescing these shared memory writes? > + } > + }; > +} > + > +/// 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: Medium] Does this unintentionally restrict the mapping to the thread that created it? Because VMap stores its mapping address as a raw pointer addr: *mut c_void, it implicitly lacks the Send and Sync traits in Rust. This means VMap and VMapOwned cannot be safely transferred or shared across concurrency boundaries. Since the kernel's virtual mapping should be globally valid as long as the underlying owner object provides safe concurrent access, should explicit unsafe impl Send and unsafe impl Sync be provided for this struct? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
