Re: [PATCH v17 3/6] rust: drm: gem: shmem: Add vmap functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing volatile memory accesses in `IoCapable` implementation for `VMap`.
- [Medium] Unconditional reservation lock acquisition causes runtime deadlocks when vmapping from locked contexts.
- [Low] Missing `// INVARIANT:` comment when constructing `VMap`.
- [Low] Incorrect safety comment referencing `ARef` in `raw_vunmap`.
--

commit c7d123d2fcea8aeccc045288b1eb2a5cc650d43c
Author: Lyude Paul <lyude@redhat.com>

rust: drm: gem: shmem: Add vmap functions

This commit adds vmap bindings for GEM shmem objects in Rust. It introduces
functions to create and manage virtual memory mappings for these objects,
including IoCapable implementations for the VMap structure.

> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index 650c34dd8b7a4..1f05a5bc5fe66 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs

[ ... ]

> @@ -197,6 +205,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>,
> +    {
> +        // INVARIANT: We check here that the gem object is at least as large as `SIZE`.
> +        if self.size() < SIZE {
> +            return Err(ENOSPC);
> +        }
> +
> +        let mut map: MaybeUninit<bindings::iosys_map> = MaybeUninit::uninit();
> +        let guard = DmaResvGuard::new(self);

[Severity: Medium]
Could unconditionally acquiring the DMA reservation lock here lead to a deadlock
if make_vmap() is called while the lock is already held? In the DRM subsystem,
it is common to vmap a GEM object while its lock is already held, such as
during a commit pipeline or inside drm_exec().

> +
> +        // SAFETY: drm_gem_shmem_vmap can be called with the DMA reservation lock held
> +        to_result(unsafe {
> +            bindings::drm_gem_shmem_vmap_locked(self.as_raw_shmem(), map.as_mut_ptr())
> +        })?;
> +
> +        // Drop the guard explicitly here, since we may need to call raw_vunmap() (which re-acquires
> +        // the lock).
> +        drop(guard);
> +
> +        // SAFETY: The call to drm_gem_shmem_vmap_locked succeeded above, so we are guaranteed that
> +        // map is properly initialized.
> +        let map = unsafe { map.assume_init() };
> +
> +        // 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 {

[Severity: Low]
Should there be an INVARIANT comment here explaining why the memory validity
invariant is satisfied? The VMap struct documentation has an Invariants section
stating that the memory remains valid until dropped, so constructing it requires
an explanation of how the invariant is met.

> +                // 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(),
> +            })
> +        }
> +    }
> +
> +    /// Unmap a vmap from the gem object.
> +    ///
> +    /// # Safety
> +    ///
> +    /// - The caller promises that `map` is a valid vmap on this gem object.
> +    /// - The caller promises that the memory pointed to by map will no longer be accesed through
> +    ///   this instance.
> +    unsafe fn raw_vunmap(&self, mut map: bindings::iosys_map) {
> +        let _guard = DmaResvGuard::new(self);

[Severity: Medium]
Will unconditionally acquiring the lock here cause a deadlock if a VMap object
is dropped while the reservation lock is already held?

> +
> +        // SAFETY:
> +        // - This function is safe to call with the DMA reservation lock held.
> +        // - Our `ARef` is proof that the underlying gem object here is initialized and thus safe to
> +        //   dereference.

[Severity: Low]
Does this safety comment incorrectly reference an ARef? The raw_vunmap function
takes a standard shared reference to self, so there is no ARef involved in its
signature, and the safety actually relies on the reference itself guaranteeing
the object is alive.

> +        unsafe { bindings::drm_gem_shmem_vunmap_locked(self.as_raw_shmem(), &mut map) };
> +    }

[ ... ]

> +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]
Since GEM shmem buffers are mapped into both CPU and GPU address spaces, can the
compiler optimize away or reorder these standard non-volatile memory accesses?

The equivalent C API uses READ_ONCE for system memory. Would using
ptr::read_volatile enforce the necessary volatile semantics to prevent
potential issues when interacting with the GPU?

> +            }
> +
> +            #[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]
Similarly to io_read, can the compiler optimize or delay this non-volatile
write?

Would using ptr::write_volatile match the WRITE_ONCE semantics from the C API
and prevent the compiler from caching values or reordering the accesses?

> +            }
> +        }
> +    };
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603195210.693856-1-lyude@redhat.com?part=3