* [PATCH] rust: drm: gem: shmem: do not leak pages when sg_table() fails
@ 2026-10-19 20:21 ` Sami Tolvanen via B4 Relay
0 siblings, 0 replies; 5+ messages in thread
From: Sami Tolvanen via B4 Relay @ 2026-10-19 20:21 UTC (permalink / raw)
To: David Airlie, Simona Vetter, Danilo Krummrich, Alice Ryhl,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Daniel Almeida,
Tamir Duberstein, Alexandre Courbot, Onur Özkan, Lyude Paul
Cc: dri-devel, rust-for-linux, linux-kernel, Sami Tolvanen
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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] rust: drm: gem: shmem: do not leak pages when sg_table() fails
2026-10-19 20:21 ` Sami Tolvanen via B4 Relay
(?)
@ 2026-10-20 12:23 ` Alice Ryhl
-1 siblings, 0 replies; 5+ messages in thread
From: Alice Ryhl @ 2026-10-20 12:23 UTC (permalink / raw)
To: Sami Tolvanen
Cc: David Airlie, Simona Vetter, Danilo Krummrich, Miguel Ojeda,
Boqun Feng, Gary Guo, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Lyude Paul, dri-devel,
rust-for-linux, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] rust: drm: gem: shmem: do not leak pages when sg_table() fails
2026-10-19 20:21 ` Sami Tolvanen via B4 Relay
(?)
(?)
@ 2026-10-20 12:31 ` Gary Guo
2026-10-20 20:16 ` Sami Tolvanen
-1 siblings, 1 reply; 5+ messages in thread
From: Gary Guo @ 2026-10-20 12:31 UTC (permalink / raw)
To: samitolvanen, David Airlie, Simona Vetter, Danilo Krummrich,
Alice Ryhl, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Trevor Gross, Daniel Almeida, Tamir Duberstein, Alexandre Courbot,
Onur Özkan, Lyude Paul
Cc: dri-devel, rust-for-linux, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] rust: drm: gem: shmem: do not leak pages when sg_table() fails
2026-10-20 12:31 ` Gary Guo
@ 2026-10-20 20:16 ` Sami Tolvanen
0 siblings, 0 replies; 5+ messages in thread
From: Sami Tolvanen @ 2026-10-20 20:16 UTC (permalink / raw)
To: Gary Guo
Cc: David Airlie, Simona Vetter, Danilo Krummrich, Alice Ryhl,
Miguel Ojeda, Boqun Feng, Björn Roy Baron, Benno Lossin,
Andreas Hindborg, Trevor Gross, Daniel Almeida, Tamir Duberstein,
Alexandre Courbot, Onur Özkan, Lyude Paul, dri-devel,
rust-for-linux, linux-kernel
On Tue, Oct 20, 2026 at 5:31 AM Gary Guo <gary@garyguo.net> wrote:
>
> 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.
Correct, SGTableMap::new() increments pages_use_count, but
__drm_gem_shmem_free_sgt_locked() doesn't decrement it in
SGTableMap::drop(). The count is only decremented when the object is
freed, in drm_gem_shmem_release(). This works unless Devres::new()
fails here and sg_table() is called again. I'm not sure if there's
some background that led to this design, but I do agree that it
probably could be more intuitive.
Sami
^ permalink raw reply [flat|nested] 5+ messages in thread