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

Deborah Brouwer
2026-04-09T22:57:38Z

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
>