Re: [PATCH v10 4/5] rust: drm: gem: Introduce shmem::SGTable

[Deborah Brouwer <deborah.brouwer@collabora.com>]

On Wed, Apr 08, 2026 at 08:12:50PM -0400, Lyude Paul wrote:
> 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. We store this in an UnsafeCell and protect
> access to it using the dma_resv lock that we already have from the shmem
> gem object, which is the same lock that currently protects
> drm_gem_object_shmem->sgt.
> 
> We also provide two different methods for acquiring an sg table:
> self.sg_table(), and self.owned_sg_table(). The first function is for
> short-term uses of mapped SGTables, the second is for callers that need to
> hold onto the mapped SGTable for an extended period of time. The second
> variant uses Devres of course, whereas the first simply relies on rust's
> borrow checker to prevent driver-unbind when using the mapped SGTable.
> 
> 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.
> 
>  rust/kernel/drm/gem/shmem.rs | 191 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 189 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index c643f18b20838..111be446213df 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs
> @@ -11,25 +11,38 @@
>  
>  use crate::{
>      container_of,
> +    device::{
> +        self,
> +        Bound, //
> +    },
> +    devres::*,
>      drm::{
>          driver,
>          gem,
>          private::Sealed, //
>          Device,
>      },
> -    error::to_result,
> +    error::{
> +        from_err_ptr, //
> +        to_result,
> +    },
>      prelude::*,
> +    scatterlist,
>      types::{
>          ARef,
>          Opaque, //
>      }, //
>  };
>  use core::{
> +    cell::UnsafeCell,
>      ops::{
>          Deref,
>          DerefMut, //
>      },
> -    ptr::NonNull,
> +    ptr::{
> +        self,
> +        NonNull, //
> +    },
>  };
>  use gem::{
>      BaseObjectPrivate,
> @@ -65,6 +78,10 @@ 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.
> +    ///
> +    /// This is protected by the object's dma_resv lock.
> +    sgt_res: UnsafeCell<Option<Devres<SGTableMap<T>>>>,
>      #[pin]
>      inner: T,
>  }
> @@ -117,6 +134,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: UnsafeCell::new(None),
>                  inner <- T::new(dev, size, args),
>              }),
>              GFP_KERNEL,
> @@ -176,6 +194,100 @@ 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) };
>      }
> +
> +    // If necessary, create an SGTable for the gem object and register a Devres for it to ensure
> +    // that it is unmapped on driver unbind.
> +    fn create_sg_table<'a>(
> +        &'a self,
> +        dev: &'a device::Device<Bound>,
> +    ) -> Result<&'a Devres<SGTableMap<T>>> {
> +        let ret;
> +        let sgt_res_ptr = self.sgt_res.get();
> +
> +        // SAFETY: This lock is initialized throughout the lifetime of the gem object
> +        unsafe { bindings::dma_resv_lock(self.raw_dma_resv(), ptr::null_mut()) };
> +
> +        // SAFETY: We just grabbed the lock required for reading this data above.
> +        let sgt_res = unsafe { (*sgt_res_ptr).as_ref() };
> +        if let Some(sgt_res) = sgt_res {
> +            // We already have a Devres object for this sg table, return it
> +            ret = Ok(sgt_res);
> +        } else {
> +            // SAFETY: We grabbed the lock required for calling this function above */
> +            let sgt = from_err_ptr(unsafe {
> +                bindings::drm_gem_shmem_get_pages_sgt_locked(self.as_raw_shmem())
> +            });
> +
> +            if let Err(e) = sgt {
> +                ret = Err(e);
> +            } else {
> +                // INVARIANT:
> +                // - We called drm_gem_shmem_get_pages_sgt_locked above and checked that it
> +                //   succeeded, fulfilling the invariant of SGTableRef that the object's `sgt` field
> +                //   is initialized.
> +                // - We store this Devres in the object itself and don't move it, ensuring that the
> +                //   object it points to remains valid for the lifetime of the SGTableRef.
> +                let devres = Devres::new(dev, init!(SGTableMap { obj: self.into() }));
> +                match devres {
> +                    Ok(devres) => {
> +                        // SAFETY: We acquired the lock protecting this data above, making it safe
> +                        // to write into here
> +                        unsafe { (*sgt_res_ptr) = Some(devres) };
> +
> +                        // SAFETY: We just write Some() into *sgt_res_ptr above
> +                        ret = Ok(unsafe { (&*sgt_res_ptr).as_ref().unwrap_unchecked() });
> +                    }
> +                    Err(e) => {
> +                        // We can't make sure that the pages for this object are unmapped on
> +                        // driver-unbind, so we need to release the sgt
> +                        // SAFETY:
> +                        // - We grabbed the lock required for calling this function above
> +                        // - We checked above that get_pages_sgt_locked() was successful
> +                        unsafe { bindings::__drm_gem_shmem_free_sgt_locked(self.as_raw_shmem()) };
> +
> +                        ret = Err(e);
> +                    }
> +                }
> +            }
> +        }
> +
> +        // SAFETY: We're releasing the lock that we grabbed above.
> +        unsafe { bindings::dma_resv_unlock(self.raw_dma_resv()) };
> +
> +        ret
> +    }
> +
> +    /// 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.
> +    #[inline]
> +    pub fn sg_table<'a>(
> +        &'a self,
> +        dev: &'a device::Device<Bound>,
> +    ) -> Result<&'a scatterlist::SGTable> {
> +        let sgt = self.create_sg_table(dev)?;
> +
> +        Ok(sgt.access(dev)?.deref())
> +    }
> +
> +    /// Creates (if necessary) and returns an owned reference to a scatter-gather table of DMA pages
> +    /// for this object.
> +    ///
> +    /// This is the same as [`sg_table`](Self::sg_table), except that it instead returns an
> +    /// [`shmem::SGTable`] which holds a reference to the associated gem object, instead of a
> +    /// reference to an [`scatterlist::SGTable`].
> +    ///
> +    /// This will pin the object in memory.
> +    ///
> +    /// [`shmem::SGTable`]: SGTable
> +    pub fn owned_sg_table(&self, dev: &device::Device<Bound>) -> Result<SGTable<T>> {
> +        self.create_sg_table(dev)?;
> +
> +        // INVARIANT: We just ensured above that `self.sgt_res` is initialized with
> +        // `Some(Devres<SGTableMap<T>>)`.
> +        Ok(SGTable(self.into()))
> +    }
>  }
>  
>  impl<T: DriverObject> Deref for Object<T> {
> @@ -226,3 +338,78 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
>          dumb_map_offset: None,
>      };
>  }
> +
> +/// 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.0.owner.sgt` has an initialized and valid 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() };
> +
> +        // SAFETY: The dma_resv for GEM objects is initialized throughout its lifetime
> +        unsafe { bindings::dma_resv_lock(obj.raw_dma_resv(), ptr::null_mut()) };
> +
> +        // SAFETY: We acquired the lock needed for calling this function above
> +        unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };

When i was testing this with tyr I got a double free here when I unloaded the module.

When our drm::Device (drm::Device<TyrDrmDriver, Ctx>) was released, its device data
(TyrDrmDeviceData) was dropped. In our device data we are indirectly holding several gem
Objects through the firmware (Firmware → sections → KernelBo → bo)

When those objects are destroyed, shmem::Object::free_callback() runs. This calls
drm_gem_shmem_release() which, on the C side frees shmem→sgt if it exists
via __drm_gem_shmem_free_sgt_locked(shmem).

But the Rust Object<T> still has the sgt_res. So, at the end of free_callback(), KBox::from_raw(this) triggers
SGTableMap::drop(), and that calls __drm_gem_shmem_free_sgt_locked() again for the same shmem object.

So the same sgt ends up being freed once from drm_gem_shmem_release() and again later from SGTableMap::drop().

For now my fix is just to clear sgt_res in free_callback() before drm_gem_shmem_release() which
prevents the sgt from being freed by the C side.

I plan to post our latest tyr series soon including this abstraction, and I hope you can have a look,
perhaps how we’ve set up the device data is unexpected.

> +
> +        // SAFETY: We are releasing the lock we acquired above.
> +        unsafe { bindings::dma_resv_unlock(obj.raw_dma_resv()) };
> +    }
> +}
> +
> +// SAFETY: The NonNull in SGTableRef 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 SGTableRef 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> {}
> +
> +/// An owned reference to a scatter-gather table of DMA address spans for a GEM shmem object.
> +///
> +/// This object holds an owned reference to the underlying GEM shmem object, ensuring that the
> +/// [`scatterlist::SGTable`] referenced by this type remains valid for the lifetime of this object.
> +///
> +/// # Invariants
> +///
> +/// - This type is proof that `self.0.sgt_res` is initialized with a `Some(Devres<SGTableMap<T>>)`.
> +/// - This object is only exposed in situations where we know the underlying `SGTable` will not be
> +///   modified for the lifetime of this object. Thus, it is safe to send/access this type across
> +///   threads.
> +pub struct SGTable<T: DriverObject>(ARef<Object<T>>);
> +
> +// SAFETY: This object is thread-safe via our type invariants.
> +unsafe impl<T: DriverObject> Send for SGTable<T> {}
> +// SAFETY: This object is thread-safe via our type invariants.
> +unsafe impl<T: DriverObject> Sync for SGTable<T> {}
> +
> +impl<T: DriverObject> Deref for SGTable<T> {
> +    type Target = Devres<SGTableMap<T>>;
> +
> +    fn deref(&self) -> &Self::Target {
> +        // SAFETY: `self.owner.sgt_res` is guaranteed to be initialized with
> +        // `Some(Devres<SGTableMap<T>>)` via our type invariants
> +        unsafe { (*self.0.sgt_res.get()).as_ref().unwrap_unchecked() }
> +    }
> +}
> -- 
> 2.53.0
>