[PATCH] rust: drm: gem: shmem: do not leak pages when sg_table() fails

[PATCH] rust: drm: gem: shmem: do not leak pages when sg_table() fails

[Sami Tolvanen via B4 Relay]

From: Sami Tolvanen <samitolvanen@google.com>

`SGTableMap::new()` takes a pages reference and maps the table before
`Devres::new()` allocates. If the allocation fails, dropping the map
frees the table but keeps the reference, and the next `sg_table()`
takes another one. `drm_gem_shmem_release()` puts only one, so the
pages leak.

Take the pages in the initializer, after the allocation.

Fixes: 616c229ab010 ("rust: drm: gem: Introduce shmem::Object::sg_table()")
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
---
 rust/kernel/drm/gem/shmem.rs | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index a687d46d170d..72e4af3b0503 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -555,15 +555,18 @@ fn drop(&mut self) {
 }
 
 impl<T: DriverObject> SGTableMap<T> {
-    fn new(obj: &Object<T>) -> impl Init<Self, Error> {
-        // INVARIANT:
-        // - We call drm_gem_shmem_get_pages_sgt 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()) })?;
-
-        Ok(Self { obj: obj.into() })
+    fn new(obj: &Object<T>) -> impl Init<Self, Error> + '_ {
+        pin_init::init_scope(move || {
+            // 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()) })?;
+
+            // INVARIANT:
+            // - We call drm_gem_shmem_get_pages_sgt above and check whether or not it succeeds,
+            //   fulfilling the invariant of SGTableMap that the object's `sgt` field is
+            //   initialized.
+            Ok(try_init!(Self { obj: obj.into() }))
+        })
     }
 }
 

---
base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
change-id: 20261019-rust-shmem-fixes-3a838ada39bf

Re: [PATCH] rust: drm: gem: shmem: do not leak pages when sg_table() fails

[Alice Ryhl]

On Mon, Oct 19, 2026 at 08:21:52PM +0000, Sami Tolvanen wrote:
> `SGTableMap::new()` takes a pages reference and maps the table before
> `Devres::new()` allocates. If the allocation fails, dropping the map
> frees the table but keeps the reference, and the next `sg_table()`
> takes another one. `drm_gem_shmem_release()` puts only one, so the
> pages leak.
> 
> Take the pages in the initializer, after the allocation.
> 
> Fixes: 616c229ab010 ("rust: drm: gem: Introduce shmem::Object::sg_table()")
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>

LGTM:

Cc: stable@vger.kernel.org
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Looks like there might still be another issue when Devres drops
SGTableMap, leaving a handle to the GEM open.

Alice

Re: [PATCH] rust: drm: gem: shmem: do not leak pages when sg_table() fails

[Gary Guo]

On Mon Oct 19, 2026 at 9:21 PM BST, Sami Tolvanen via B4 Relay wrote:
> From: Sami Tolvanen <samitolvanen@google.com>
>
> `SGTableMap::new()` takes a pages reference and maps the table before
> `Devres::new()` allocates. If the allocation fails, dropping the map
> frees the table but keeps the reference, and the next `sg_table()`
> takes another one. `drm_gem_shmem_release()` puts only one, so the
> pages leak.

Sorry, could you explain it in more detail?

Creating a SGTableMap and dropping it doesn't restore the state? If so, it
sounds like an API design issue.

Best,
Gary

>
> Take the pages in the initializer, after the allocation.
>
> Fixes: 616c229ab010 ("rust: drm: gem: Introduce shmem::Object::sg_table()")
> Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
> ---
>  rust/kernel/drm/gem/shmem.rs | 21 ++++++++++++---------
>  1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
> index a687d46d170d..72e4af3b0503 100644
> --- a/rust/kernel/drm/gem/shmem.rs
> +++ b/rust/kernel/drm/gem/shmem.rs
> @@ -555,15 +555,18 @@ fn drop(&mut self) {
>  }
>  
>  impl<T: DriverObject> SGTableMap<T> {
> -    fn new(obj: &Object<T>) -> impl Init<Self, Error> {
> -        // INVARIANT:
> -        // - We call drm_gem_shmem_get_pages_sgt 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()) })?;
> -
> -        Ok(Self { obj: obj.into() })
> +    fn new(obj: &Object<T>) -> impl Init<Self, Error> + '_ {
> +        pin_init::init_scope(move || {
> +            // 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()) })?;
> +
> +            // INVARIANT:
> +            // - We call drm_gem_shmem_get_pages_sgt above and check whether or not it succeeds,
> +            //   fulfilling the invariant of SGTableMap that the object's `sgt` field is
> +            //   initialized.
> +            Ok(try_init!(Self { obj: obj.into() }))
> +        })
>      }
>  }
>  
>
> ---
> base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
> change-id: 20261019-rust-shmem-fixes-3a838ada39bf