[PATCH 0/3] Fix import of dma-buf buffers as DRM GEM shmem objects

[PATCH 0/3] Fix import of dma-buf buffers as DRM GEM shmem objects

[Janne Grunau]

Hej,

this is triggered by the discussion of gem/shmem.rs SGTableMap issues
in [1] which has an additional issue with imported object.
Imported objects are supported since GEM shmem's AllocOps use
drm_gem_shmem_prime_import_sg_table() as gem_prime_import_sg_table which
sets the same function pointer field in struct drm_driver.

The code is broken in two ways. SGTableMap::drop() calls
__drm_gem_shmem_free_sgt_locked() uncondiftionally which it must not do
for imported objects as the object does not own `sgt`.
The second issue is that the imported object is allocated by
__drm_gem_shmem_create() with sizeof(struct drm_gem_shmem_object) as
size instead of the size of Rust's Object<T>.

The second issue is already fixed in tyr's downstream driver by adding a
`gem_create_object` callback which is used instead of the minimal
allocation in the C helper.
The first issue is fixed in the Asahi downstream driver together with a
drm_gem_is_imported() wrapper change taken from the tyr downstream
driver.

All three patches are tested in the Asahi downstream driver. As this is
unused code in-tree I marked the fixes as not suitable for stable
backports. The known downstream users of the code are aware of the
issues and carry fixes downstream. I don't expect new Rust DRM drivers
to be based on the (old) stable trees.

Link: https://lore.kernel.org/rust-for-linux/20261019-rust-shmem-fixes-v1-1-3f7f6de5b5e7@google.com/ [1]
Signed-off-by: Janne Grunau <j@jannau.net>
---
Janne Grunau (1):
      rust: drm: gem: shmem: Do not free SG table of imported objects

Sami Tolvanen (2):
      rust: drm: gem: shmem: implement gem_create_object for prime imports
      rust: drm: gem: add an is_imported() accessor

 drivers/gpu/drm/nova/gem.rs  |  4 +++
 drivers/gpu/drm/tyr/gem.rs   |  4 +++
 rust/helpers/drm.c           |  6 +++++
 rust/kernel/drm/gem/mod.rs   | 16 ++++++++++++
 rust/kernel/drm/gem/shmem.rs | 62 +++++++++++++++++++++++++++++++++++++++++++-
 5 files changed, 91 insertions(+), 1 deletion(-)
---
base-commit: 3497a403264b094ebd61b8b755165d56e7d9dc37
change-id: 20261026-rust-drm-gem-shmem-imported-sgt-85c4a45be283

Best regards,
-- 
Janne Grunau <j@jannau.net>

[PATCH 1/3] rust: drm: gem: shmem: implement gem_create_object for prime imports

[Janne Grunau]

From: Sami Tolvanen <samitolvanen@google.com>

For a prime-imported BO with no `gem_create_object` callback,
`__drm_gem_shmem_create()` allocates `kzalloc(sizeof(struct
drm_gem_shmem_object))`, which is smaller than the Rust `Object<T>`
wrapper. A later `container_of!()` from a `drm_gem_object` back to
`Object<T>` then runs past the allocation and corrupts the heap.

Provide a `gem_create_object` callback that allocates the full
`Object<T>` via `KBox::pin_init`. Move `obj` to the first field so the
returned pointer is at offset zero, satisfying the `kfree()` on the
init-failure path. This is now compile-checked with `const_assert!`.

Imported BOs have no userspace creation arguments, so extend
`DriverObject` with a `create_imported(dev, size)` constructor and
implement it for Tyr's `BoData`, Nova, and the in-tree KUnit driver.

Fixes: 80df573af9ef ("rust: drm: gem: shmem: Add DRM shmem helper abstraction")
Cc: <stable+noautosel@kernel.org> # no in tree users
Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Janne Grunau <j@jannau.net>
---
 drivers/gpu/drm/nova/gem.rs  |  4 +++
 drivers/gpu/drm/tyr/gem.rs   |  4 +++
 rust/kernel/drm/gem/mod.rs   |  9 +++++++
 rust/kernel/drm/gem/shmem.rs | 58 +++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nova/gem.rs b/drivers/gpu/drm/nova/gem.rs
index 2b6fe9dc0bfa..2e17422e76f5 100644
--- a/drivers/gpu/drm/nova/gem.rs
+++ b/drivers/gpu/drm/nova/gem.rs
@@ -27,6 +27,10 @@ impl gem::DriverObject for NovaObject {
     fn new(_dev: &NovaDevice, _size: usize, _args: Self::Args) -> impl PinInit<Self, Error> {
         try_pin_init!(NovaObject {})
     }
+
+    fn create_imported(_dev: &NovaDevice, _size: usize) -> impl PinInit<Self, Error> {
+        try_pin_init!(NovaObject {})
+    }
 }
 
 impl NovaObject {
diff --git a/drivers/gpu/drm/tyr/gem.rs b/drivers/gpu/drm/tyr/gem.rs
index 3bf3787f5c3f..4b61bb04c845 100644
--- a/drivers/gpu/drm/tyr/gem.rs
+++ b/drivers/gpu/drm/tyr/gem.rs
@@ -48,6 +48,10 @@ impl gem::DriverObject for BoData {
     fn new(_dev: &TyrDrmDevice, _size: usize, args: BoCreateArgs) -> impl PinInit<Self, Error> {
         try_pin_init!(Self { flags: args.flags })
     }
+
+    fn create_imported(_dev: &TyrDrmDevice, _size: usize) -> impl PinInit<Self, Error> {
+        try_pin_init!(Self { flags: 0 })
+    }
 }
 
 /// Type alias for Tyr GEM buffer objects.
diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
index e1ebad77ebe2..e01b8311dd89 100644
--- a/rust/kernel/drm/gem/mod.rs
+++ b/rust/kernel/drm/gem/mod.rs
@@ -98,6 +98,15 @@ fn new(
         args: Self::Args,
     ) -> impl PinInit<Self, Error>;
 
+    /// Create a new driver data object for a GEM object that is being imported
+    /// from another driver (e.g. via dma-buf).
+    ///
+    /// Imported objects are allocated by the importing helper, which only
+    /// provides the device and size. There is no driver-supplied
+    /// [`DriverObject::Args`], so drivers must initialize the per-object
+    /// state without those arguments.
+    fn create_imported(dev: &drm::Device<Self::Driver>, size: usize) -> impl PinInit<Self, Error>;
+
     /// Open a new handle to an existing object, associated with a File.
     fn open(_obj: &DriverAllocImpl<Self>, _file: &DriverFile<Self>) -> Result {
         Ok(())
diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index a687d46d170d..1df3554893cc 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -100,6 +100,12 @@ fn default() -> Self {
 #[repr(C)]
 #[pin_data]
 pub struct Object<T: DriverObject> {
+    /// Embedded `drm_gem_shmem_object`.
+    ///
+    /// Must remain the first field so the allocation address coincides with the embedded
+    /// `drm_gem_shmem_object`. The DRM shmem helper's `__drm_gem_shmem_create()` error path calls
+    /// `kfree()` on the returned `drm_gem_object` pointer, which is only sound when it points at
+    /// the start of the allocation.
     #[pin]
     obj: Opaque<bindings::drm_gem_shmem_object>,
     /// Parent object that owns this object's DMA reservation object.
@@ -343,6 +349,49 @@ pub fn new(
         Ok(obj)
     }
 
+    /// Allocates the full [`Object<T>`] wrapper and zero-initializes the embedded shmem object.
+    /// The helper's `__drm_gem_shmem_init` fills it in after we return.
+    extern "C" fn gem_create_object_callback(
+        raw_dev: *mut bindings::drm_device,
+        size: usize,
+    ) -> *mut bindings::drm_gem_object {
+        const_assert!(
+            core::mem::offset_of!(Self, obj) == 0,
+            "Object<T>::obj must be at offset 0 so drm_gem_shmem_helper's kfree(obj) error path \
+             frees the wrapper allocation, not the middle of it"
+        );
+
+        // SAFETY: The DRM shmem helper only invokes this callback for a registered DRM device of
+        // type `T::Driver`, which is `Self`'s driver per the `AllocImpl` impl below.
+        let dev: &Device<T::Driver> = unsafe { Device::from_raw(raw_dev) };
+
+        let new: Pin<KBox<Self>> = match KBox::try_pin_init(
+            try_pin_init!(Self {
+                obj <- Opaque::init_zeroed(),
+                parent_resv_obj: None,
+                sgt_res: ManuallyDrop::new(SetOnce::new()),
+                sgt_lock <- new_mutex!(()),
+                inner <- T::create_imported(dev, size),
+            }),
+            GFP_KERNEL,
+        ) {
+            Ok(new) => new,
+            Err(e) => return e.to_ptr(),
+        };
+
+        // SAFETY: `new.as_raw()` is guaranteed to be valid by the initialization above.
+        unsafe { (*new.as_raw()).funcs = &Self::VTABLE };
+
+        // SAFETY: We never move out of `self`. Ownership of the allocation is transferred to the
+        // C side, which will run `__drm_gem_shmem_init()` next and later free us via
+        // `free_callback` (or `kfree()` on the `__drm_gem_shmem_init()` error path, matching the
+        // C kzalloc fallback contract).
+        let new = KBox::into_raw(unsafe { Pin::into_inner_unchecked(new) });
+
+        // SAFETY: `new` was just produced by `KBox::into_raw` and so is a valid pointer to an
+        // initialized `Self`. `obj.base` is the embedded `drm_gem_object` the caller expects.
+        unsafe { &raw mut (*(*new).obj.get()).base }
+    }
     /// Creates and returns an owned reference to a virtual kernel memory mapping for this object.
     #[inline]
     pub fn owned_vmap<const SIZE: usize>(&self) -> Result<VMapOwned<T, SIZE>> {
@@ -389,7 +438,7 @@ impl<T: DriverObject> driver::AllocImpl for Object<T> {
     type Driver = T::Driver;
 
     const ALLOC_OPS: driver::AllocOps = driver::AllocOps {
-        gem_create_object: None,
+        gem_create_object: Some(Self::gem_create_object_callback),
         prime_handle_to_fd: None,
         prime_fd_to_handle: None,
         gem_prime_import: None,
@@ -623,6 +672,13 @@ fn new(
         ) -> impl PinInit<Self, Error> {
             try_pin_init!(KunitObject {})
         }
+
+        fn create_imported(
+            _dev: &drm::Device<KunitDriver>,
+            _size: usize,
+        ) -> impl PinInit<Self, Error> {
+            try_pin_init!(KunitObject {})
+        }
     }
 
     #[vtable]

-- 
2.56.0

[PATCH 2/3] rust: drm: gem: add an is_imported() accessor

[Janne Grunau]

From: Sami Tolvanen <samitolvanen@google.com>

Drivers need to check whether a GEM object is PRIME-imported.

Add BaseObject::is_imported(), wrapping drm_gem_is_imported() through a
C helper shim.

Signed-off-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Janne Grunau <j@jannau.net>
---
 rust/helpers/drm.c         | 6 ++++++
 rust/kernel/drm/gem/mod.rs | 7 +++++++
 2 files changed, 13 insertions(+)

diff --git a/rust/helpers/drm.c b/rust/helpers/drm.c
index 65f3f22b0e1d..8cf3ca7aeeb2 100644
--- a/rust/helpers/drm.c
+++ b/rust/helpers/drm.c
@@ -16,6 +16,12 @@ __rust_helper void rust_helper_drm_gem_object_put(struct drm_gem_object *obj)
 	drm_gem_object_put(obj);
 }
 
+__rust_helper bool
+rust_helper_drm_gem_is_imported(const struct drm_gem_object *obj)
+{
+	return drm_gem_is_imported(obj);
+}
+
 __rust_helper __u64
 rust_helper_drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
 {
diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
index e01b8311dd89..5da8627275a2 100644
--- a/rust/kernel/drm/gem/mod.rs
+++ b/rust/kernel/drm/gem/mod.rs
@@ -187,6 +187,13 @@ fn size(&self) -> usize {
         unsafe { (*self.as_raw()).size }
     }
 
+    /// Returns whether the object's backing buffer is imported from another
+    /// driver.
+    fn is_imported(&self) -> bool {
+        // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `struct drm_gem_object`.
+        unsafe { bindings::drm_gem_is_imported(self.as_raw()) }
+    }
+
     /// Creates a new handle for the object associated with a given `File`
     /// (or returns an existing one).
     fn create_handle<D, F>(&self, file: &drm::File<F>) -> Result<u32>

-- 
2.56.0

[PATCH 3/3] rust: drm: gem: shmem: Do not free SG table of imported objects

[Janne Grunau]

GEM shmem objects imported through gem_prime_import_sg_table wrapping
drm_gem_shmem_prime_import_sg_table() do not own their SG table.
SGTableMap::drop() must not free it since it will be freed on exporter
side once the DMAbuf is not used anymore.

Fixes: 616c229ab010 ("rust: drm: gem: Introduce shmem::Object::sg_table()")
Cc: <stable+noautosel@kernel.org> # no in-tree users
Signed-off-by: Janne Grunau <j@jannau.net>
---
 rust/kernel/drm/gem/shmem.rs | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/rust/kernel/drm/gem/shmem.rs b/rust/kernel/drm/gem/shmem.rs
index 1df3554893cc..d0f89ad2202f 100644
--- a/rust/kernel/drm/gem/shmem.rs
+++ b/rust/kernel/drm/gem/shmem.rs
@@ -596,9 +596,13 @@ impl<T: DriverObject> Drop for SGTableMap<T> {
     fn drop(&mut self) {
         // SAFETY: `obj` is always valid via our type invariants
         let obj = unsafe { self.obj.as_ref() };
+        if obj.is_imported() {
+            return;
+        }
         let _lock = DmaResvGuard::new(obj);
 
         // SAFETY: We acquired the lock needed for calling this function above
+        // and the GEM object is not imported.
         unsafe { bindings::__drm_gem_shmem_free_sgt_locked(obj.as_raw_shmem()) };
     }
 }

-- 
2.56.0

Re: [PATCH 2/3] rust: drm: gem: add an is_imported() accessor

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] rust: drm: gem: missing #[inline] on BaseObject::is_imported()
--

commit 870fd74aba497ab1671a05a6bf7eca2012bd1ed7
Author: Sami Tolvanen <samitolvanen@google.com>

rust: drm: gem: add an is_imported() accessor

This commit adds BaseObject::is_imported() and a corresponding C helper
shim to check whether a DRM GEM object is PRIME-imported.

> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index e01b8311dd895..5da8627275a29 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs
> @@ -187,6 +187,13 @@ fn size(&self) -> usize {
>          unsafe { (*self.as_raw()).size }
>      }
>  
> +    /// Returns whether the object's backing buffer is imported from another
> +    /// driver.
> +    fn is_imported(&self) -> bool {
> +        // SAFETY: `self.as_raw()` is guaranteed to be a pointer to a valid `struct drm_gem_object`.
> +        unsafe { bindings::drm_gem_is_imported(self.as_raw()) }
> +    }

[Severity: Low]
This isn't a bug, but since BaseObject::is_imported() in
rust/kernel/drm/gem/mod.rs is a small abstraction method in the kernel
crate that directly forwards to bindings::drm_gem_is_imported(), should it
be annotated with #[inline] per the Rust coding guidelines?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-rust-drm-gem-shmem-imported-sgt-v1-0-36f15ace04b1@jannau.net?part=2