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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] `SGTableMap::drop` unconditionally frees the scatter/gather table, which corrupts memory for imported dma-buf GEM objects.
- [Critical] Concurrent device unbind and GEM object destruction causes a Use-After-Free of the GEM object and its dma_resv lock in `SGTableMap::drop`.
- [High] `SGTableMap` fails to pin the shmem GEM object backing pages, exposing the SG table to being purged by the shrinker and causing a NULL pointer dereference. It also leaks the `pages_use_count` on drop.
--

commit 8870abbe2654a7715fcd51335a14b62f57e22680
Author: Lyude Paul <lyude@redhat.com>
rust: drm: gem: Introduce shmem::Object::sg_table()

This commit introduces a Devres-managed SGTableMap to ensure that
scatterlist mappings for GEM shmem objects are released upon driver
unbind. It adds the sg_table() method to shmem::Object which is intended
to pin the object and return its scatter/gather table.

> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index 1f05a5bc5fe66..dff90771eb34a 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs
[ ... ]
> @@ -189,18 +210,25 @@ 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, C>`
> -        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 drop `sgt_res` first, since doing so requires that the GEM object is still
> +        // alive.
> +        // SAFETY:
> +        // - We verified above that `this` is valid.
> +        // - We are in free_callback, guaranteeing we have exclusive access to `this`.
> +        unsafe { &mut (*this).sgt_res }.reset();

[Severity: Critical]
Does this leave a window for a use-after-free if the device unbinds
concurrently?

If the devres subsystem is already running SGTableMap::drop() in another
thread during device unbind, sgt_res.reset() could silently fail to remove
the action. The callback would then proceed to free the GEM object while
SGTableMap::drop() is still dereferencing and locking it.

> +
> +        // 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) };
[ ... ]
> +impl<T: DriverObject, C: DeviceContext> Drop for SGTableMap<T, C> {
> +    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()) };

[Severity: Critical]
Is it safe to unconditionally free the scatter/gather table here?

If the GEM object was imported from a dma-buf, the scatter/gather table
belongs to the exporter. Calling __drm_gem_shmem_free_sgt_locked() directly
without checking drm_gem_is_imported() seems like it might illegally free
the exporter's memory.

[Severity: High]
Could this drop implementation also leak the pages_use_count?

Calling __drm_gem_shmem_free_sgt_locked() alone doesn't seem to drop the
page use count acquired during SGTableMap::new(). Should this also call
drm_gem_shmem_put_pages_locked() to balance the counts?

> +    }
> +}
> +
> +impl<T: DriverObject, C: DeviceContext> SGTableMap<T, C> {
> +    fn new(obj: &Object<T, C>) -> 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()) })?;

[Severity: High]
Does this expose the object to being purged by the DRM LRU shrinker?

The Object::sg_table() documentation mentions this will pin the object in
memory, but drm_gem_shmem_get_pages_sgt() only increments the
pages_use_count, not the pages_pin_count. If userspace marks the object as
MADV_DONTNEED, the shrinker could purge it and set shmem->sgt to NULL.

Could this lead to a NULL pointer dereference later in SGTableMap::drop()
when it passes the pointer to __drm_gem_shmem_free_sgt_locked()?

> +
> +        Ok(Self { obj: obj.into() })
> +    }
> +}

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