[PATCH v15 0/6] Rust bindings for gem shmem

[PATCH v15 0/6] Rust bindings for gem shmem

[Lyude Paul]

Most of this patch series has already been pushed upstream, this is just
the second half of the patch series that has not been pushed yet + some
additional changes which were required to implement changes requested by
the mailing list. This patch series is originally from Asahi, previously
posted by Daniel Almeida.

The previous version of the patch series can be found here:

	https://patchwork.freedesktop.org/series/164580/

Branch with patches applied available here:

	https://gitlab.freedesktop.org/lyudess/linux/-/commits/rust/gem-shmem

This patch series applies on top of drm-rust-next with the following
dependencies applied:

	https://lore.kernel.org/rust-for-linux/20260529173137.303717-1-lyude@redhat.com/T/#t

Lyude Paul (6):
  rust: gem: shmem: Fix Default implementation for ObjectConfig
  rust: drm: gem/shmem: Add DmaResvGuard helper
  rust: drm: gem: Add vmap functions to shmem bindings
  rust: faux: Allow retrieving a bound Device
  drm/gem/shmem: Introduce __drm_gem_shmem_free_sgt_locked()
  rust: drm: gem: Introduce shmem::Object::sg_table()

 drivers/gpu/drm/drm_gem_shmem_helper.c |  32 +-
 include/drm/drm_gem_shmem_helper.h     |   1 +
 rust/kernel/drm/gem/shmem.rs           | 497 ++++++++++++++++++++++++-
 rust/kernel/faux.rs                    |   7 +-
 4 files changed, 512 insertions(+), 25 deletions(-)


base-commit: 0e42ec83d46ab8877d38d37493328ed7d1a24de8
prerequisite-patch-id: c8ade07eec6e9c9e875800b114137c459d362e4e
prerequisite-patch-id: c38da06dbc4cfd2589bf2e7e9f0ebaa4da521ddf
-- 
2.54.0

[PATCH v15 1/6] rust: gem: shmem: Fix Default implementation for ObjectConfig

[Lyude Paul]

I completely forgot when coming up with this type that #[derive(Default)]
only works if all generics mentioned in the type implement Default (and T
usually doesn't). This being said: We don't use `T` for anything besides
using it for a reference type, so whether or not it implements `Default`
shouldn't actually need to matter.

So, fix this by just manually implementing Default instead of deriving it.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 rust/kernel/drm/gem/shmem.rs | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index e1b648920d2f6..8b7de136ab1f9 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -39,7 +39,6 @@
 ///
 /// This is used with [`Object::new()`] to control various properties that can only be set when
 /// initially creating a shmem-backed GEM object.
-#[derive(Default)]
 pub struct ObjectConfig<'a, T: DriverObject> {
     /// Whether to set the write-combine map flag.
     pub map_wc: bool,
@@ -50,6 +49,16 @@ pub struct ObjectConfig<'a, T: DriverObject> {
     pub parent_resv_obj: Option<&'a Object<T>>,
 }
 
+impl<'a, T: DriverObject> Default for ObjectConfig<'a, T> {
+    #[inline(always)]
+    fn default() -> Self {
+        Self {
+            map_wc: false,
+            parent_resv_obj: None,
+        }
+    }
+}
+
 /// A shmem-backed GEM object.
 ///
 /// # Invariants
-- 
2.54.0

Re: [PATCH v15 1/6] rust: gem: shmem: Fix Default implementation for ObjectConfig

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The `default()` method in the manual `Default` implementation for `ObjectConfig` is incorrectly annotated with `#[inline(always)]` instead of `#[inline]`.
--

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

rust: gem: shmem: Fix Default implementation for ObjectConfig

This commit replaces the derived Default implementation for ObjectConfig with
a manual implementation. This resolves an issue where the derive macro
incorrectly requires the generic type T to implement Default even though it
is only used as a reference.

> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index e1b648920d2f6..8b7de136ab1f9 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs
[ ... ]
> @@ -50,6 +49,16 @@ pub struct ObjectConfig<'a, T: DriverObject> {
>      pub parent_resv_obj: Option<&'a Object<T>>,
>  }
>  
> +impl<'a, T: DriverObject> Default for ObjectConfig<'a, T> {
> +    #[inline(always)]

[Severity: Low]
This isn't a bug, but should this be #[inline] instead of #[inline(always)]?

According to the Rust subsystem coding guidelines, #[inline(always)] is
reserved for functions using build_assert!() that depend on function
parameters. Small abstraction functions and default method implementations
like this should generally use #[inline] instead.

> +    fn default() -> Self {
> +        Self {
> +            map_wc: false,
> +            parent_resv_obj: None,
> +        }
> +    }
> +}

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

[PATCH v15 2/6] rust: drm: gem/shmem: Add DmaResvGuard helper

[Lyude Paul]

Just a temporary holdover to make locking/unlocking the dma_resv lock much
easier.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Co-authored-by: Alexandre Courbot <acourbot@nvidia.com>
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 rust/kernel/drm/gem/shmem.rs | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 8b7de136ab1f9..a116b0752e46a 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -27,7 +27,10 @@
         Deref,
         DerefMut, //
     },
-    ptr::NonNull,
+    ptr::{
+        self,
+        NonNull, //
+    },
 };
 use gem::{
     BaseObjectPrivate,
@@ -233,3 +236,29 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
         dumb_map_offset: None,
     };
 }
+
+/// Private helper-type for holding the `dma_resv` object for a GEM shmem object.
+///
+/// When this is dropped, the `dma_resv` lock is dropped as well.
+///
+// TODO: This should be replace with a WwMutex equivalent once we have such bindings in the kernel.
+struct DmaResvGuard<'a, T: DriverObject>(&'a Object<T>);
+
+impl<'a, T: DriverObject> DmaResvGuard<'a, T> {
+    #[inline(always)]
+    #[expect(unused)]
+    fn new(obj: &'a Object<T>) -> Self {
+        // SAFETY: This lock is initialized throughout the lifetime of `object`.
+        unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
+
+        Self(obj)
+    }
+}
+
+impl<'a, T: DriverObject> Drop for DmaResvGuard<'a, T> {
+    #[inline(always)]
+    fn drop(&mut self) {
+        // SAFETY: We are releasing the lock grabbed during the creation of this object.
+        unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) };
+    }
+}
-- 
2.54.0

Re: [PATCH v15 2/6] rust: drm: gem/shmem: Add DmaResvGuard helper

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] DmaResvGuard implicitly implements Send, violating kernel mutex semantics.
- [Low] Incorrect usage of `#[inline(always)]` instead of `#[inline]` on abstraction methods.
--

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

rust: drm: gem/shmem: Add DmaResvGuard helper

This commit introduces a temporary DmaResvGuard helper struct to make
locking and unlocking the dma_resv lock for GEM shmem objects easier.

> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index 8b7de136ab1f9..a116b0752e46a 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs

[ ... ]

> @@ -233,3 +236,29 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
>          dumb_map_offset: None,
>      };
>  }
> +
> +/// Private helper-type for holding the `dma_resv` object for a GEM shmem object.
> +///
> +/// When this is dropped, the `dma_resv` lock is dropped as well.
> +///
> +// TODO: This should be replace with a WwMutex equivalent once we have such bindings in the kernel.
> +struct DmaResvGuard<'a, T: DriverObject>(&'a Object<T>);

[Severity: High]
Does this implicitly implement the Send trait if Object<T> implements Sync?

Since this guard drops a kernel ww_mutex in its Drop implementation, moving
the guard to a different thread and unlocking it there would violate the
kernel mutex API contracts. The task that acquires the lock must be the same
task that releases it, otherwise it triggers a kernel warning or panic.

Should this struct include a PhantomData<*mut ()> or
kernel::types::NotThreadSafe marker to explicitly opt out of being Send?

> +
> +impl<'a, T: DriverObject> DmaResvGuard<'a, T> {
> +    #[inline(always)]

[Severity: Low]
This isn't a bug, but does this require the inline(always) attribute?

According to the Rust subsystem coding guidelines, inline(always) should
only be used for functions utilizing build_assert!() that depend on
function parameters. Small abstractions that merely forward to C bindings
should use the standard inline attribute.

> +    #[expect(unused)]
> +    fn new(obj: &'a Object<T>) -> Self {
> +        // SAFETY: This lock is initialized throughout the lifetime of `object`.
> +        unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
> +
> +        Self(obj)
> +    }
> +}
> +
> +impl<'a, T: DriverObject> Drop for DmaResvGuard<'a, T> {
> +    #[inline(always)]

[Severity: Low]
This isn't a bug, but similarly here, could this use the standard inline
attribute instead of inline(always)?

> +    fn drop(&mut self) {
> +        // SAFETY: We are releasing the lock grabbed during the creation of this object.
> +        unsafe { bindings::dma_resv_unlock(self.0.raw_dma_resv()) };
> +    }
> +}

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

[PATCH v15 3/6] rust: drm: gem: Add vmap functions to shmem bindings

[Lyude Paul]

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.

Similar to how we handle SGTables, we make sure there's two different types
of mappings: owned mappings (kernel::drm::gem::shmem::VMap) and borrowed
mappings (kernel::drm::gem::shmem::VMapRef).

Signed-off-by: Lyude Paul <lyude@redhat.com>

---
V7:
* Switch over to the new iosys map bindings that use the Io trait
V8:
* Get rid of iosys_map bindings for now, only support non-iomem types
* s/as_shmem()/as_raw_shmem()
V9:
* Get rid of some outdated comments I missed
* Add missing SIZE check to raw_vmap()
* Add a proper unit test that ensures that we actually validate SIZE at
  compile-time.
  Turns out it takes only 34 lines to make a boilerplate DRM driver for a
  kunit test :)
* Add unit tests
* Add some missing #[inline]s
V10:
* Correct issue with iomem error path
  We previously called raw_vunmap() if we got an iomem allocation, but
  raw_vunmap() was written such that it assumed all allocations were sysmem
  allocations. Fix this by just making raw_vunmap() accept a iosys_map.
V11:
* Use Alexandre's clever solution to remove the macros we were using for
  maintaining two different VMap types.
* Change the order of items in Object<T> to ensure that sgt_res is always
  dropped before obj.
* Fix typo in Object.raw_vmap()
* s/raw_vmap()/make_vmap()/
  Deduplicate code a bit more as well by using more generics here
V15:
* Add these patches back
* We only have one VMap type now!
* Use ObjectConfig::default() in unit tests since we unbroke it.

 rust/kernel/drm/gem/shmem.rs | 433 +++++++++++++++++++++++++++++++++--
 1 file changed, 418 insertions(+), 15 deletions(-)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index a116b0752e46a..494e0d0d8d0d6 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -11,18 +11,39 @@
 
 use crate::{
     container_of,
+    device::{
+        self,
+        Bound, //
+    },
+    devres::*,
     drm::{
-        device,
         driver,
         gem,
-        private::Sealed, //
+        private::Sealed,
+        Device, //
+    },
+    error::{
+        from_err_ptr,
+        to_result, //
+    },
+    io::{
+        Io,
+        IoCapable,
+        IoKnownSize, //
     },
-    error::to_result,
     prelude::*,
-    sync::aref::ARef,
+    scatterlist,
+    sync::{
+        aref::ARef,
+        new_lazy_init,
+        LazyInit,
+        LazyInitError, //
+    },
     types::Opaque, //
 };
 use core::{
+    ffi::c_void,
+    mem::MaybeUninit, //
     ops::{
         Deref,
         DerefMut, //
@@ -33,6 +54,7 @@
     },
 };
 use gem::{
+    BaseObject,
     BaseObjectPrivate,
     DriverObject,
     IntoGEMObject, //
@@ -75,6 +97,9 @@ pub struct Object<T: DriverObject> {
     obj: Opaque<bindings::drm_gem_shmem_object>,
     /// Parent object that owns this object's DMA reservation object.
     parent_resv_obj: Option<ARef<Object<T>>>,
+    /// Devres object for unmapping any SGTable on driver-unbind.
+    #[pin]
+    sgt_res: LazyInit<Devres<SGTableMap<T>>>,
     #[pin]
     inner: T,
 }
@@ -118,7 +143,7 @@ fn as_raw_shmem(&self) -> *mut bindings::drm_gem_shmem_object {
     ///
     /// Additional config options can be specified using `config`.
     pub fn new(
-        dev: &device::Device<T::Driver>,
+        dev: &Device<T::Driver>,
         size: usize,
         config: ObjectConfig<'_, T>,
         args: T::Args,
@@ -127,6 +152,7 @@ pub fn new(
             try_pin_init!(Self {
                 obj <- Opaque::init_zeroed(),
                 parent_resv_obj: config.parent_resv_obj.map(|p| p.into()),
+                sgt_res <- new_lazy_init!(),
                 inner <- T::new(dev, size, args),
             }),
             GFP_KERNEL,
@@ -160,9 +186,9 @@ pub fn new(
     }
 
     /// Returns the `Device` that owns this GEM object.
-    pub fn dev(&self) -> &device::Device<T::Driver> {
+    pub fn dev(&self) -> &Device<T::Driver> {
         // SAFETY: `dev` will have been initialized in `Self::new()` by `drm_gem_shmem_init()`.
-        unsafe { device::Device::from_raw((*self.as_raw()).dev) }
+        unsafe { Device::from_raw((*self.as_raw()).dev) }
     }
 
     extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
@@ -170,22 +196,123 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
         // - DRM always passes a valid gem object here
         // - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
         //   `obj` is contained within a drm_gem_shmem_object
-        let this = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
-
-        // SAFETY:
-        // - We're in free_callback - so this function is safe to call.
-        // - We won't be using the gem resources on `this` after this call.
-        unsafe { bindings::drm_gem_shmem_release(this) };
+        let base = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
 
         // SAFETY:
         // - We verified above that `obj` is valid, which makes `this` valid
         // - This function is set in AllocOps, so we know that `this` is contained within a
         //   `Object<T>`
-        let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut();
+        let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut();
+
+        // We need to reset `sgt_res` first, since doing so requires that the GEM object is still
+        // alive.
+        // SAFETY:
+        // - We verified above that `this` is valid.
+        // - `&mut (*this)` does not implement Unpin.
+        unsafe { Pin::new_unchecked(&mut (*this).sgt_res) }.reset();
+
+        // SAFETY:
+        // - We're in free_callback - so this function is safe to call.
+        // - We won't be using the gem resources on `this` after this call.
+        unsafe { bindings::drm_gem_shmem_release(base) };
 
         // 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, 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);
+
+        // 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 {
+                // 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);
+
+        // 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.
+        unsafe { bindings::drm_gem_shmem_vunmap_locked(self.as_raw_shmem(), &mut map) };
+    }
+
+    /// Creates and returns a virtual kernel memory mapping for this object.
+    #[inline]
+    pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, SIZE>> {
+        self.make_vmap()
+    }
+
+    /// Creates and returns an owned reference to a virtual kernel memory mapping for this object.
+    #[inline]
+    pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, SIZE>> {
+        self.make_vmap()
+    }
+
+    /// Creates (if necessary) and returns an immutable reference to a scatter-gather table of DMA
+    /// pages for this object.
+    ///
+    /// This will pin the object in memory. It is expected that `dev` should be a pointer to the
+    /// same [`device::Device`] which `self` belongs to, otherwise this function will return
+    /// `Err(EINVAL)`.
+    pub fn sg_table<'a>(
+        &'a self,
+        dev: &'a device::Device<Bound>,
+    ) -> Result<&'a scatterlist::SGTable> {
+        if dev.as_raw() != self.dev().as_ref().as_raw() {
+            return Err(EINVAL);
+        }
+
+        match self.sgt_res.init(Devres::new(dev, SGTableMap::new(self))) {
+            Ok(ret) | Err(LazyInitError::AlreadyInit(ret)) => Ok(ret.access(dev)?),
+            Err(LazyInitError::DuringInit(e)) => Err(e),
+        }
+    }
 }
 
 impl<T: DriverObject> Deref for Object<T> {
@@ -246,7 +373,6 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
 
 impl<'a, T: DriverObject> DmaResvGuard<'a, T> {
     #[inline(always)]
-    #[expect(unused)]
     fn new(obj: &'a Object<T>) -> Self {
         // SAFETY: This lock is initialized throughout the lifetime of `object`.
         unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
@@ -262,3 +388,280 @@ 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, const SIZE: usize> IoCapable<$ty> for $impl<D, R, SIZE>
+        where
+            D: DriverObject,
+            R: Deref<Target = Object<D>>,
+        {
+            #[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) }
+            }
+
+            #[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) }
+            }
+        }
+    };
+}
+
+/// 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, const SIZE: usize = 0>
+where
+    D: DriverObject,
+    R: Deref<Target = Object<D>>,
+{
+    addr: *mut c_void,
+    owner: R,
+}
+
+/// An alias type for a reference to a shmem-based GEM object's VMap.
+pub type VMapRef<'a, D, const SIZE: usize = 0> = VMap<D, &'a Object<D>, SIZE>;
+
+/// An alias type for an owned reference to a shmem-based GEM object's VMap.
+pub type VMapOwned<D, const SIZE: usize = 0> = VMap<D, ARef<Object<D>>, SIZE>;
+
+impl<D, R, const SIZE: usize> VMap<D, R, SIZE>
+where
+    D: DriverObject,
+    R: Deref<Target = Object<D>>,
+{
+    /// Borrows a reference to the object that owns this virtual mapping.
+    #[inline(always)]
+    pub fn owner(&self) -> &Object<D> {
+        &self.owner
+    }
+}
+
+impl<D, R, const SIZE: usize> Drop for VMap<D, R, SIZE>
+where
+    D: DriverObject,
+    R: Deref<Target = Object<D>>,
+{
+    #[inline(always)]
+    fn drop(&mut self) {
+        // SAFETY:
+        // - Our existence is proof that this map was previously created using self.owner.
+        // - Since we are in Drop, we are guaranteed that no one will access the memory
+        //   through this mapping after calling this.
+        unsafe {
+            self.owner.raw_vunmap(bindings::iosys_map {
+                is_iomem: false,
+                __bindgen_anon_1: bindings::iosys_map__bindgen_ty_1 { vaddr: self.addr },
+            })
+        };
+    }
+}
+
+impl<D, R, const SIZE: usize> Io for VMap<D, R, SIZE>
+where
+    D: DriverObject,
+    R: Deref<Target = Object<D>>,
+{
+    #[inline(always)]
+    fn addr(&self) -> usize {
+        self.addr as usize
+    }
+
+    #[inline(always)]
+    fn maxsize(&self) -> usize {
+        self.owner.size()
+    }
+}
+
+impl<D, R, const SIZE: usize> IoKnownSize for VMap<D, R, SIZE>
+where
+    D: DriverObject,
+    R: Deref<Target = Object<D>>,
+{
+    const MIN_SIZE: usize = SIZE;
+}
+
+impl_vmap_io_capable!(VMap, u8);
+impl_vmap_io_capable!(VMap, u16);
+impl_vmap_io_capable!(VMap, u32);
+#[cfg(CONFIG_64BIT)]
+impl_vmap_io_capable!(VMap, u64);
+
+/// A reference to a GEM object that is known to have a mapped [`SGTable`].
+///
+/// This is used by the Rust bindings with [`Devres`] in order to ensure that mappings for SGTables
+/// on GEM shmem objects are revoked on driver-unbind.
+///
+/// # Invariants
+///
+/// - `self.obj` always points to a valid GEM object.
+/// - This object is proof that `self.obj.owner.sgt` has an initialized and valid
+///   [`scatterlist::SGTable`].
+pub struct SGTableMap<T: DriverObject> {
+    obj: NonNull<Object<T>>,
+}
+
+impl<T: DriverObject> Deref for SGTableMap<T> {
+    type Target = scatterlist::SGTable;
+
+    fn deref(&self) -> &Self::Target {
+        // SAFETY:
+        // - The NonNull is guaranteed to be valid via our type invariants.
+        // - The sgt field is guaranteed to be initialized and valid via our type invariants.
+        unsafe { scatterlist::SGTable::from_raw((*self.obj.as_ref().as_raw_shmem()).sgt) }
+    }
+}
+
+impl<T: DriverObject> Drop for SGTableMap<T> {
+    fn drop(&mut self) {
+        // SAFETY: `obj` is always valid via our type invariants
+        let obj = unsafe { self.obj.as_ref() };
+        let _lock = DmaResvGuard::new(obj);
+
+        // SAFETY: We acquired the lock needed for calling this function above
+        unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };
+    }
+}
+
+impl<T: DriverObject> SGTableMap<T> {
+    fn new(obj: &Object<T>) -> impl Init<Self, Error> {
+        // INVARIANT:
+        // - We call drm_gem_shmem_get_pages_sgt_locked below and check whether or not it
+        //   succeeds, fulfilling the invariant of SGTableMap that the object's `sgt` field is
+        //   initialized.
+        // SAFETY:
+        // - `obj` is fully initialized, making this function safe to call.
+        from_err_ptr(unsafe { bindings::drm_gem_shmem_get_pages_sgt(obj.as_raw_shmem()) })?;
+
+        Ok(Self { obj: obj.into() })
+    }
+}
+
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject> Send for SGTableMap<T> {}
+// SAFETY: The NonNull in SGTableMap is guaranteed valid by our type invariants, and the GEM object
+// it points to is guaranteed to be thread-safe.
+unsafe impl<T: DriverObject> Sync for SGTableMap<T> {}
+
+#[kunit_tests(rust_drm_gem_shmem)]
+mod tests {
+    use super::*;
+    use crate::{
+        drm,
+        faux,
+        page::PAGE_SIZE, //
+    };
+
+    // The bare minimum needed to create a fake drm driver for kunit
+
+    #[pin_data]
+    struct KunitData {}
+    struct KunitDriver;
+    struct KunitFile;
+    #[pin_data]
+    struct KunitObject {}
+
+    const INFO: drm::DriverInfo = drm::DriverInfo {
+        major: 0,
+        minor: 0,
+        patchlevel: 0,
+        name: c"kunit",
+        desc: c"Kunit",
+    };
+
+    impl drm::file::DriverFile for KunitFile {
+        type Driver = KunitDriver;
+
+        fn open(_dev: &drm::Device<KunitDriver>) -> Result<Pin<KBox<Self>>> {
+            Ok(KBox::new(Self, GFP_KERNEL)?.into())
+        }
+    }
+
+    impl gem::DriverObject for KunitObject {
+        type Driver = KunitDriver;
+        type Args = ();
+
+        fn new(
+            _dev: &drm::Device<KunitDriver>,
+            _size: usize,
+            _args: Self::Args,
+        ) -> impl PinInit<Self, Error> {
+            try_pin_init!(KunitObject {})
+        }
+    }
+
+    #[vtable]
+    impl drm::Driver for KunitDriver {
+        type Data = KunitData;
+        type File = KunitFile;
+        type Object = Object<KunitObject>;
+
+        const INFO: drm::DriverInfo = INFO;
+        const IOCTLS: &'static [drm::ioctl::DrmIoctlDescriptor] = &[];
+    }
+
+    fn create_drm_dev() -> Result<(faux::Registration, ARef<drm::Device<KunitDriver>>)> {
+        // Create a faux DRM device so we can test gem object creation.
+        let data = try_pin_init!(KunitData {});
+        let dev = faux::Registration::new(c"Kunit", None)?;
+        let drm = drm::Device::<KunitDriver>::new(dev.as_ref(), data)?;
+
+        Ok((dev, drm))
+    }
+
+    #[test]
+    fn compile_time_vmap_sizes() -> Result {
+        let (_dev, drm) = create_drm_dev()?;
+
+        let obj = Object::<KunitObject>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+        // Try creating a normal vmap
+        obj.vmap::<PAGE_SIZE>()?;
+
+        // Try creating a vmap that's smaller then the size we specified
+        obj.vmap::<{ PAGE_SIZE - 100 }>()?;
+
+        // Make sure creating a vmap that's too large fails
+        assert!(obj.vmap::<{ PAGE_SIZE + 200 }>().is_err());
+
+        Ok(())
+    }
+
+    #[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(0xFFFFFFFF, 0x20);
+
+        assert_eq!(vmap.read32(0x20), 0xFFFFFFFF);
+
+        assert_eq!(vmap.read8(0x20), 0xFF);
+        assert_eq!(vmap.read8(0x21), 0xFF);
+        assert_eq!(vmap.read8(0x22), 0xFF);
+        assert_eq!(vmap.read8(0x23), 0xFF);
+
+        Ok(())
+    }
+}
-- 
2.54.0

Re: [PATCH v15 3/6] rust: drm: gem: Add vmap functions to shmem bindings

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Calling `Object::sg_table()` multiple times sequentially or concurrently causes a Use-After-Free and Double Free of the GEM object's SG table due to eager evaluation.
- [High] `SGTableMap::drop` can access the `Object` after it has been freed if the device is unbound concurrently with the GEM object's destruction (Use-After-Free).
- [High] The `IoCapable` implementation for `VMap` uses non-volatile memory accesses, enabling dangerous compiler optimizations on memory shared with hardware.
- [Medium] Using uninterruptible lock `DmaResvGuard` for `vmap` operations.
- [Low] The patch introduces a call to an undefined C function `__drm_gem_shmem_free_sgt_locked`, breaking bisectability.
- [Low] Returning `ENOTSUPP` instead of `EOPNOTSUPP` for unsupported I/O memory allocations.
--

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

rust: drm: gem: Add vmap functions to shmem bindings

This commit adds vmap bindings for gem shmem objects in Rust, introducing
owned and borrowed mappings similar to SGTables.

> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index a116b0752e46a..494e0d0d8d0d6 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs

[ ... ]

> @@ -170,22 +196,123 @@ extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
>          // - We verified above that `obj` is valid, which makes `this` valid
>          // - This function is set in AllocOps, so we know that `this` is contained within a
>          //   `Object<T>`
> -        let this = unsafe { container_of!(Opaque::cast_from(this), Self, obj) }.cast_mut();
> +        let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut();
> +
> +        // We need to reset `sgt_res` first, since doing so requires that the GEM object is still
> +        // alive.
> +        // SAFETY:
> +        // - We verified above that `this` is valid.
> +        // - `&mut (*this)` does not implement Unpin.
> +        unsafe { Pin::new_unchecked(&mut (*this).sgt_res) }.reset();

[Severity: High]
If the device is unbound concurrently with the GEM object's destruction, does
this reset() allow the devres callback to access the Object after it has
been freed? Since the devres callback might already be executing and has
extracted the value, reset() could complete without waiting, proceeding to
free the Object memory while SGTableMap::drop later dereferences the freed
pointer.

> +
> +        // SAFETY:
> +        // - We're in free_callback - so this function is safe to call.
> +        // - We won't be using the gem resources on `this` after this call.
> +        unsafe { bindings::drm_gem_shmem_release(base) };
>  
>          // 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, 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]
Is it safe to use the uninterruptible DmaResvGuard::new() here? This is a
user-triggerable path (creating a vmap), and acquiring the dma_resv lock
uninterruptibly might cause unkillable tasks during heavy lock contention.
The C helper drm_gem_shmem_vmap uses the interruptible version.

> +
> +        // 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)

[Severity: Low]
Should this return EOPNOTSUPP instead? ENOTSUPP is an internal kernel
error code that shouldn't be returned to userspace, as it can cause
unexpected behavior in libc if propagated through an ioctl.

> +        } 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(),
> +            })
> +        }
> +    }

[ ... ]

> +    pub fn sg_table<'a>(
> +        &'a self,
> +        dev: &'a device::Device<Bound>,
> +    ) -> Result<&'a scatterlist::SGTable> {
> +        if dev.as_raw() != self.dev().as_ref().as_raw() {
> +            return Err(EINVAL);
> +        }
> +
> +        match self.sgt_res.init(Devres::new(dev, SGTableMap::new(self))) {

[Severity: Critical]
Could this lead to a double-free of the SG table? In Rust, function
arguments are evaluated eagerly. SGTableMap::new(self) executes
unconditionally before LazyInit::init checks if it is already initialized.

If sgt_res is already initialized, init() will return AlreadyInit, and the
newly instantiated SGTableMap argument will be dropped. The Drop impl for
SGTableMap unconditionally frees the shared shmem->sgt, which would leave
the first SGTableMap instance holding a dangling reference.

> +            Ok(ret) | Err(LazyInitError::AlreadyInit(ret)) => Ok(ret.access(dev)?),
> +            Err(LazyInitError::DuringInit(e)) => Err(e),
> +        }
> +    }
>  }

[ ... ]

> +macro_rules! impl_vmap_io_capable {
> +    ($impl:ident, $ty:ty) => {
> +        impl<D, R, const SIZE: usize> IoCapable<$ty> for $impl<D, R, SIZE>
> +        where
> +            D: DriverObject,
> +            R: Deref<Target = Object<D>>,
> +        {
> +            #[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]
Does using standard non-volatile ptr::read() enable dangerous compiler
optimizations here? The memory mapped via a GEM object is shared with
hardware and is often mapped as Write-Combine. Standard accesses might
allow the compiler to hoist reads out of loops or split reads. The C
implementation uses READ_ONCE() to prevent this.

> +            }
> +
> +            #[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, does ptr::write() risk the compiler eliminating stores it
deems dead? Using WRITE_ONCE() would ensure the memory accesses happen
exactly as specified for hardware-shared memory.

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

[ ... ]

> +impl<T: DriverObject> Drop for SGTableMap<T> {
> +    fn drop(&mut self) {
> +        // SAFETY: `obj` is always valid via our type invariants
> +        let obj = unsafe { self.obj.as_ref() };

[Severity: High]
If the devres callback executes this concurrently with
Object::free_callback resetting the sgt_res, is it possible that this
dereferences a raw pointer to an Object that has just been freed?

> +        let _lock = DmaResvGuard::new(obj);
> +
> +        // SAFETY: We acquired the lock needed for calling this function above
> +        unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };

[Severity: Low]
Does this break bisectability? The function
__drm_gem_shmem_free_sgt_locked() does not seem to be defined in the C
headers at this commit.

> +    }
> +}

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

[PATCH v15 4/6] rust: faux: Allow retrieving a bound Device

[Lyude Paul]

When writing up some rust code that used faux devices for unit testing, I
noticed that we never actually added the Bound device context to
faux::Registration's AsRef<device::Device> implementation. This being said:
the Registration object itself is proof that a driver is bound to the
device - so this should be safe.

Signed-off-by: Lyude Paul <lyude@redhat.com>
---
 rust/kernel/faux.rs | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs
index 43b4974f48cd2..e0856b2964a2c 100644
--- a/rust/kernel/faux.rs
+++ b/rust/kernel/faux.rs
@@ -25,7 +25,8 @@
 ///
 /// # Invariants
 ///
-/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`].
+/// - `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`].
+/// - This object is proof that the object described by this `Registration` is bound to a device.
 ///
 /// [`struct faux_device`]: srctree/include/linux/device/faux.h
 pub struct Registration(NonNull<bindings::faux_device>);
@@ -59,8 +60,8 @@ fn as_raw(&self) -> *mut bindings::faux_device {
     }
 }
 
-impl AsRef<device::Device> for Registration {
-    fn as_ref(&self) -> &device::Device {
+impl AsRef<device::Device<device::Bound>> for Registration {
+    fn as_ref(&self) -> &device::Device<device::Bound> {
         // SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be
         // a valid initialized `device`.
         unsafe { device::Device::from_raw(addr_of_mut!((*self.as_raw()).dev)) }
-- 
2.54.0

Re: [PATCH v15 4/6] rust: faux: Allow retrieving a bound Device

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The newly added `Registration` invariant lacks an `// INVARIANT:` justification at construction, contains a semantic typo, and the corresponding `// SAFETY:` comment in `as_ref()` was not updated to justify the stricter `Device<Bound>` context.
--

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

rust: faux: Allow retrieving a bound Device

This commit updates faux::Registration to implement AsRef for the stricter
Device<Bound> context rather than the default Device. It also adds a new
invariant to document that Registration is proof of the bound state.

> diff --git a/rust/kernel/faux.rs b/rust/kernel/faux.rs
> index 43b4974f48cd2..e0856b2964a2c 100644
> --- a/rust/kernel/faux.rs
> +++ b/rust/kernel/faux.rs
> @@ -25,7 +25,8 @@
>  ///
>  /// # Invariants
>  ///
> -/// `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`].
> +/// - `self.0` always holds a valid pointer to an initialized and registered [`struct faux_device`].
> +/// - This object is proof that the object described by this `Registration` is bound to a device.

[Severity: Medium]
This isn't a bug, but there appears to be a semantic typo in the invariant 
wording. The documentation states "bound to a device", but the Registration
object represents the device itself. Could this be corrected to "bound to a
driver" to match the commit message?

Also, since a new invariant was added to Registration, does
Registration::new() need an updated // INVARIANT: comment to explain why
this new invariant is satisfied at construction?

According to the Rust subsystem guidelines, when a struct has an Invariants
section, its construction site should document why the invariants hold:

rust/kernel/faux.rs:Registration::new() {
    ...
    // INVARIANT: The device will remain registered until faux_device_destroy()
    // is called, which happens in our Drop implementation.
    Self(ptr)
}

>  ///
>  /// [`struct faux_device`]: srctree/include/linux/device/faux.h
>  pub struct Registration(NonNull<bindings::faux_device>);

[ ... ]

> @@ -59,8 +60,8 @@ fn as_raw(&self) -> *mut bindings::faux_device {
>      }
>  }
>  
> -impl AsRef<device::Device> for Registration {
> -    fn as_ref(&self) -> &device::Device {
> +impl AsRef<device::Device<device::Bound>> for Registration {
> +    fn as_ref(&self) -> &device::Device<device::Bound> {
>          // SAFETY: The underlying `device` in `faux_device` is guaranteed by the C API to be
>          // a valid initialized `device`.
>          unsafe { device::Device::from_raw(addr_of_mut!((*self.as_raw()).dev)) }

[Severity: Medium]
Does the // SAFETY: comment here need to be updated to justify why returning
the stricter Device<Bound> context is valid?

The current comment justifies instantiating a Device from a raw pointer, but
does not explain how the caller is guaranteed that the device is actually in
the Bound state, which the device abstraction delegates to the caller.

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

[PATCH v15 5/6] drm/gem/shmem: Introduce __drm_gem_shmem_free_sgt_locked()

[Lyude Paul]

One of the complications of trying to use the shmem helpers to create a
scatterlist for shmem objects is that we need to be able to provide a
guarantee that the driver cannot be unbound for the lifetime of the
scatterlist.

The easiest way of handling this seems to be just hooking up an unmap
operation to devres the first time we create a scatterlist, which allows us
to still take advantage of gem shmem facilities without breaking that
guarantee. To allow for this, we extract __drm_gem_shmem_free_sgt_locked()
- which allows a caller (e.g. the rust bindings) to manually unmap the sgt
for a gem object as needed.

Signed-off-by: Lyude Paul <lyude@redhat.com>
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

---
V10:
* Fix incorrect function name in documentation for
  __drm_gem_shmem_release_sgt_locked()

 drivers/gpu/drm/drm_gem_shmem_helper.c | 32 +++++++++++++++++++++-----
 include/drm/drm_gem_shmem_helper.h     |  1 +
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 545933c7f7121..c989459eb2159 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -158,6 +158,30 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
 
+/**
+ * __drm_gem_shmem_release_sgt_locked - Unpin and DMA unmap pages, and release the
+ * cached scatter/gather table for an shmem GEM object.
+ * @shmem: shmem GEM object
+ *
+ * If the passed shmem object has an active scatter/gather table for driver
+ * usage, this function will unmap it and release the memory associated with it.
+ * It is the responsibility of the caller to ensure it holds the dma_resv_lock
+ * for this object.
+ *
+ * Drivers should not need to call this function themselves, it is mainly
+ * intended for usage in the Rust shmem bindings.
+ */
+void __drm_gem_shmem_free_sgt_locked(struct drm_gem_shmem_object *shmem)
+{
+	dma_resv_assert_held(shmem->base.resv);
+
+	dma_unmap_sgtable(shmem->base.dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0);
+	sg_free_table(shmem->sgt);
+	kfree(shmem->sgt);
+	shmem->sgt = NULL;
+}
+EXPORT_SYMBOL_GPL(__drm_gem_shmem_free_sgt_locked);
+
 /**
  * drm_gem_shmem_release - Release resources associated with a shmem GEM object.
  * @shmem: shmem GEM object
@@ -176,12 +200,8 @@ void drm_gem_shmem_release(struct drm_gem_shmem_object *shmem)
 
 		drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count));
 
-		if (shmem->sgt) {
-			dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
-					  DMA_BIDIRECTIONAL, 0);
-			sg_free_table(shmem->sgt);
-			kfree(shmem->sgt);
-		}
+		if (shmem->sgt)
+			__drm_gem_shmem_free_sgt_locked(shmem);
 		if (shmem->pages)
 			drm_gem_shmem_put_pages_locked(shmem);
 
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index 5ccdae21b94a9..b2c23af628e1a 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -111,6 +111,7 @@ int drm_gem_shmem_init(struct drm_device *dev, struct drm_gem_shmem_object *shme
 struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
 void drm_gem_shmem_release(struct drm_gem_shmem_object *shmem);
 void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem);
+void __drm_gem_shmem_free_sgt_locked(struct drm_gem_shmem_object *shmem);
 
 void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem);
 int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem);
-- 
2.54.0

Re: [PATCH v15 5/6] drm/gem/shmem: Introduce __drm_gem_shmem_free_sgt_locked()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] `__drm_gem_shmem_free_sgt_locked` does not check if the GEM object is imported, corrupting memory by unmapping and freeing an alien scatterlist owned by the dma-buf exporter.
--

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

drm/gem/shmem: Introduce __drm_gem_shmem_free_sgt_locked()

This commit extracts the scatterlist unmapping and freeing logic into a new
exported helper, __drm_gem_shmem_free_sgt_locked(). This is intended to allow
callers, such as the Rust shmem bindings, to manually unmap the sgt for a
GEM object as needed.

> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 545933c7f7121..c989459eb2159 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c

[ ... ]

> @@ -158,6 +158,30 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t
>  }
>  EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>  
> +/**
> + * __drm_gem_shmem_release_sgt_locked - Unpin and DMA unmap pages, and release the
> + * cached scatter/gather table for an shmem GEM object.
> + * @shmem: shmem GEM object
> + *
> + * If the passed shmem object has an active scatter/gather table for driver
> + * usage, this function will unmap it and release the memory associated with it.
> + * It is the responsibility of the caller to ensure it holds the dma_resv_lock
> + * for this object.
> + *
> + * Drivers should not need to call this function themselves, it is mainly
> + * intended for usage in the Rust shmem bindings.
> + */
> +void __drm_gem_shmem_free_sgt_locked(struct drm_gem_shmem_object *shmem)
> +{
> +	dma_resv_assert_held(shmem->base.resv);
> +
> +	dma_unmap_sgtable(shmem->base.dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0);
> +	sg_free_table(shmem->sgt);
> +	kfree(shmem->sgt);
> +	shmem->sgt = NULL;
> +}
> +EXPORT_SYMBOL_GPL(__drm_gem_shmem_free_sgt_locked);

[Severity: Critical]
Does this helper need to check if the GEM object is imported?

If __drm_gem_shmem_free_sgt_locked() is called unconditionally on an imported
GEM object (such as from the Rust bindings dropping an SGTableMap), it seems
like it will unmap and free the scatterlist that is actually owned by the
dma-buf exporter. 

In the original code within drm_gem_shmem_release(), this memory was protected
by an explicit check:

	if (drm_gem_is_imported(obj)) {
		drm_prime_gem_destroy(obj, shmem->sgt);
	} else {
		...
		if (shmem->sgt)
			__drm_gem_shmem_free_sgt_locked(shmem);

Can bypassing the drm_gem_is_imported() check here lead to memory corruption
or double-free issues when the exporter also tries to clean up the table?

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

[PATCH v15 6/6] rust: drm: gem: Introduce shmem::Object::sg_table()

[Lyude Paul]

In order to do this, we need to be careful to ensure that any interface we
expose for scatterlists ensures that any mappings created from one are
destroyed on driver-unbind. To do this, we introduce a Devres resource into
shmem::Object that we use in order to ensure that we release any SGTable
mappings on driver-unbind.

There's some other slightly unfortunate caveats of this:

* Drivers don't have explicit control at the moment over when unmapping
  happens (which is exactly the same as the C side atm, so it might not be
  a problem).
* We can't just return `SGTableMap` to the user through an Arc to attempt
  to fix the last caveat - because that implies the gem object would need
  to hold a reference count to the scatterlist mapping, which just leaves
  us with the same problem.

Signed-off-by: Lyude Paul <lyude@redhat.com>

---
V3:
* Rename OwnedSGTable to shmem::SGTable. Since the current version of the
  SGTable abstractions now has a `Owned` and `Borrowed` variant, I think
  renaming this to shmem::SGTable makes things less confusing.
  We do however, keep the name of owned_sg_table() as-is.
V4:
* Clarify safety comments for SGTable to explain why the object is
  thread-safe.
* Rename from SGTableRef to SGTable
V10:
* Use Devres in order to ensure that SGTables are revocable, and are
  unmapped on driver-unbind.
V11:
* s/create_sg_table()/get_sg_table()
* Get rid of extraneous `ret = ` in shmem::Object::get_sg_table()
V12:
* Actually move sgt_res in this patch and not the next one
V13:
* Use DmaResvGuard suggestion from Alexander
* Use Alexander's (much better) solution for get_sg_table()
* Use SetOnce instead of UnsafeCell
* s/SGTableRef/SGTableMap
* Fix typo in SGTableMap documentation
* Create fallible constructor for SGTableMap
* Don't reuse dma_resv lock for protecting Object contents, just use Mutex
  + SetOnce
* Drop use of drm_gem_shmem_get_pages_sgt_locked(), since we don't need to
  hold the dma_resv lock ourselves for anything but this function.
* Check that the device we receive in the bounds for sg_table() and
  owned_sg_table() that said Device is in fact, the correct device.
* Remove redundant docs in owned_sg_table(), just point it back to
  sg_table().
* Implement Deborah's suggestion to fix double-free in
  free_callback()
* Restore original order of Object<T>
* Fix doc typo for SGTableMap
V14:
* Use new InitOnce container over the Mutex/SetOnce horror show we had
  before.
* Start using LazyInit container for storing Devres for sgt unmap
* Add some kunit tests for sg_table (not sure why I didn't do this before)
  using some of the boilerplate code leftover from the vmap bindings
* Get rid of the owned SGTable variant for now, we'll add it back in a
  future patch if people actually need it.
* Use new LazyInit container from me to get rid of the horrid
  Mutex<SetOnce<>> mess.
* Add the best we can do for unit tests w/r/t SGTable at the moment

 rust/kernel/drm/gem/shmem.rs | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 494e0d0d8d0d6..5655c2a1ae8fb 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -664,4 +664,28 @@ fn vmap_io() -> Result {
 
         Ok(())
     }
+
+    // TODO: I would love to actually test the success paths of sg_table(), but that would require
+    // also implementing dummy dma_ops so that trying to create a mapping doesn't explode. So, leave
+    // that for someone else.
+
+    // Ensures that passing the wrong device to sg_table() fails as we expect, and also ensure it
+    // skips initializing `sgt_res` since we could otherwise create `sgt_res` with the wrong device
+    // bound to it.
+    #[test]
+    fn fail_sg_table_on_wrong_dev() -> Result {
+        let (_dev, drm) = create_drm_dev()?;
+        let wrong_dev = faux::Registration::new(c"EvilKunit", None)?;
+
+        let obj = Object::<KunitObject>::new(&drm, PAGE_SIZE, ObjectConfig::default(), ())?;
+
+        assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+        // If sgt_res was not initialized mistakenly with the wrong device, this should still fail.
+        assert_eq!(obj.sg_table(wrong_dev.as_ref()).err().unwrap(), EINVAL);
+
+        // TODO: Someday, we should test that creating an sg_table here still succeeds.
+
+        Ok(())
+    }
 }
-- 
2.54.0

Re: [PATCH v15 6/6] rust: drm: gem: Introduce shmem::Object::sg_table()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Eager evaluation of `Devres::new` and `SGTableMap::new` inside `sg_table()` causes a use-after-free of the scatterlist and leaking of page references.
- [High] Concurrent device unbind and GEM object free causes Use-After-Free of the GEM object in `SGTableMap::drop`.
- [Low] The patch imports and uses `LazyInit`, `new_lazy_init`, and `LazyInitError` from `crate::sync`, but these are neither defined nor exported anywhere in the tree.
--

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

rust: drm: gem: Introduce shmem::Object::sg_table()

In order to do this, we need to be careful to ensure that any interface we
expose for scatterlists ensures that any mappings created from one are
destroyed on driver-unbind. To do this, we introduce a Devres resource into
shmem::Object that we use in order to ensure that we release any SGTable
mappings on driver-unbind.

> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index 494e0d0d8d0d6..5655c2a1ae8fb 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs

[ ... ]

>      sync::{
>          aref::ARef,
>          new_lazy_init,
>          LazyInit,
>          LazyInitError, //
>      },

[Severity: Low]
Are these primitives defined somewhere else?  The patch imports and uses
LazyInit, new_lazy_init, and LazyInitError from crate::sync, but these don't
seem to be present in the tree or introduced anywhere in this patch series.
Will this lead to a module resolution failure and compilation error?

[ ... ]

>      extern "C" fn free_callback(obj: *mut bindings::drm_gem_object) {
>          // SAFETY:
>          // - DRM always passes a valid gem object here
>          // - We used drm_gem_shmem_create() in our create_gem_object callback, so we know that
>          //   `obj` is contained within a drm_gem_shmem_object
>          let base = unsafe { container_of!(obj, bindings::drm_gem_shmem_object, base) };
>  
>          // SAFETY:
>          // - We verified above that `obj` is valid, which makes `this` valid
>          // - This function is set in AllocOps, so we know that `this` is contained within a
>          //   `Object<T>`
>          let this = unsafe { container_of!(Opaque::cast_from(base), Self, obj) }.cast_mut();
>  
>          // We need to reset `sgt_res` first, since doing so requires that the GEM object is still
>          // alive.
>          // SAFETY:
>          // - We verified above that `this` is valid.
>          // - `&mut (*this)` does not implement Unpin.
>          unsafe { Pin::new_unchecked(&mut (*this).sgt_res) }.reset();

[Severity: High]
Does this code introduce a race condition causing a use-after-free?

If drm_gem_object_put() triggers free_callback() concurrently with a device
unbind, the device unbind thread's devres release callback calls
Revocable::revoke().  This swaps is_available to false and blocks on
synchronize_rcu() before dropping SGTableMap.

Concurrently, free_callback() calls sgt_res.reset(), which drops Devres.
Devres::drop() calls revoke_nosync(), which sees is_available is already false
and returns immediately without waiting for the unbind thread.

Then free_callback() proceeds to call drm_gem_shmem_release() and frees the GEM
object memory.

When the unbind thread finishes synchronize_rcu() and executes
SGTableMap::drop(), could it dereference the freed GEM object memory
to lock its dma_resv?

>  
>          // SAFETY:
>          // - We're in free_callback - so this function is safe to call.
>          // - We won't be using the gem resources on `this` after this call.
>          unsafe { bindings::drm_gem_shmem_release(base) };

[ ... ]

>      pub fn sg_table<'a>(
>          &'a self,
>          dev: &'a device::Device<Bound>,
>      ) -> Result<&'a scatterlist::SGTable> {
>          if dev.as_raw() != self.dev().as_ref().as_raw() {
>              return Err(EINVAL);
>          }
>  
>          match self.sgt_res.init(Devres::new(dev, SGTableMap::new(self))) {

[Severity: High]
Can eager evaluation cause a use-after-free and leaked page references here?

In Rust, function arguments are evaluated eagerly.  Devres::new allocates and
executes its initializer eagerly.  SGTableMap::new(self) evaluates its
initializer, causing drm_gem_shmem_get_pages_sgt() to be called synchronously
every time sg_table() is executed, incrementing the page use count.

If sgt_res is already initialized, init() returns LazyInitError::AlreadyInit
and drops the newly created Devres argument.

When Devres is dropped, it drops the inner SGTableMap, calling
SGTableMap::drop(). This calls __drm_gem_shmem_free_sgt_locked(), which
unconditionally unmaps and frees the shared shmem->sgt memory and sets it to
NULL without decrementing the page use count.

Does this leave the originally cached SGTableMap holding a reference to a NULL
or freed shmem->sgt, which would cause a NULL pointer dereference or
use-after-free the next time it's accessed?

>              Ok(ret) | Err(LazyInitError::AlreadyInit(ret)) => Ok(ret.access(dev)?),
>              Err(LazyInitError::DuringInit(e)) => Err(e),
>          }
>      }
>  }

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