From: sashiko-bot@kernel.org
Subject: Re: [PATCH v18 4/4] rust: drm: gem: Introduce
 shmem::Object::sg_table()
Reply-To: sashiko-reviews@lists.linux.dev
To: "Lyude Paul" <lyude@redhat.com>
Cc: linux-media@vger.kernel.org, ojeda@kernel.org, dri-devel@lists.freedesktop.org
In-Reply-To: <20260604192740.659240-5-lyude@redhat.com>
References: <20260604192740.659240-5-lyude@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Thu, 04 Jun 2026 19:54:17 +0000
Message-Id: <20260604195417.88EEA1F00893@smtp.kernel.org>
Precedence: bulk

Thank you for your contribution! Sashiko AI review found 1 potential issue(=
s) to consider:
- [Medium] SGTableMap::drop frees the scatter-gather table without decremen=
ting 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<VMap=
Ref<'_, 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 s=
catter-gather table of DMA
> +    /// pages for this object.
> +    ///
> +    /// This will pin the object in memory. It is expected that `dev` sh=
ould 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() !=3D self.dev().as_ref().as_raw() {
> +            return Err(EINVAL);
> +        }
> +
> +        let sgt_res =3D '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 =3D unsafe { self.obj.as_ref() };
> +        let _lock =3D 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_sh=
mem()) };

[Severity: Medium]
Does this code leak the backing pages?

This function frees the SGT and sets shmem->sgt =3D NULL, but it omits a pa=
ired
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 =3D=3D NULL and increment pages_use_count a second time. When th=
e GEM
object is eventually released, drm_gem_shmem_release() will only decrement =
it
once, failing to reach 0 and triggering a drm_WARN_ON.

> +    }
> +}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260604192740.6592=
40-1-lyude@redhat.com?part=3D4