Re: [PATCH v18 4/4] rust: drm: gem: Introduce shmem::Object::sg_table()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] SGTableMap::drop frees the scatter-gather table without decrementing the backing pages reference count, causing a permanent memory leak if Devres initialization fails.
--

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

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

This commit introduces sg_table() to shmem::Object and utilizes a Devres
resource to ensure SGTable mappings are released on driver-unbind.

> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index a20b0c0dac6bd..f3c893c7fbf4f 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs

[ ... ]

> @@ -270,6 +302,45 @@ pub fn vmap<const SIZE: usize>(&self) -> Result<VMapRef<'_, T, C, SIZE>> {
>      pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, C, 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);
> +        }
> +
> +        let sgt_res = 'out: {

[ ... ]

> +            // If not, finish initializing and return.
> +            self.sgt_res
> +                .populate(Devres::new(dev, SGTableMap::new(self))?);

[Severity: Medium]
If Devres::new() returns an error here, will the newly created SGTableMap
be immediately dropped?

When SGTableMap::new() calls drm_gem_shmem_get_pages_sgt(), it pins the
backing pages by incrementing pages_use_count and caches the new sgt. If the
SGTableMap is dropped during this error path, it frees the SGT without
decrementing pages_use_count, leading to a permanent memory leak of the
backing pages.

> +
> +            // SAFETY: We just populated sgt_res above.
> +            unsafe { self.sgt_res.as_ref().unwrap_unchecked() }
> +        };

[ ... ]

> +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: Medium]
Does this code leak the backing pages?

This function frees the SGT and sets shmem->sgt = NULL, but it omits a paired
call to drm_gem_shmem_put_pages_locked(). If this drop is triggered early
(such as when Devres::new() fails in Object::sg_table()), pages_use_count is
permanently incremented.

If the caller retries sg_table() after the failure, it will see
shmem->sgt == NULL and increment pages_use_count a second time. When the GEM
object is eventually released, drm_gem_shmem_release() will only decrement it
once, failing to reach 0 and triggering a drm_WARN_ON.

> +    }
> +}

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