Re: [PATCH v18 2/4] rust: drm: gem: shmem: Add vmap functions

[sashiko-bot@kernel.org]

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 <lyude@redhat.com>

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/20260604192740.659240-1-lyude@redhat.com?part=2