[PATCH] drm/gem: Acquire references on GEM handles for framebuffers
Christian König
christian.koenig at amd.com
Fri Jun 27 12:33:41 UTC 2025
On 27.06.25 10:20, Thomas Zimmermann wrote:
> A GEM handle can be released while the GEM buffer object is attached
> to a DRM framebuffer. This leads to the release of the dma-buf backing
> the buffer object, if any. [1] Trying to use the framebuffer in further
> mode-setting operations leads to a segmentation fault. Most easily
> happens with driver that use shadow planes for vmap-ing the dma-buf
> during a page flip. An example is shown below.
>
> [ 156.791968] ------------[ cut here ]------------
> [ 156.796830] WARNING: CPU: 2 PID: 2255 at drivers/dma-buf/dma-buf.c:1527 dma_buf_vmap+0x224/0x430
> [...]
> [ 156.942028] RIP: 0010:dma_buf_vmap+0x224/0x430
> [ 157.043420] Call Trace:
> [ 157.045898] <TASK>
> [ 157.048030] ? show_trace_log_lvl+0x1af/0x2c0
> [ 157.052436] ? show_trace_log_lvl+0x1af/0x2c0
> [ 157.056836] ? show_trace_log_lvl+0x1af/0x2c0
> [ 157.061253] ? drm_gem_shmem_vmap+0x74/0x710
> [ 157.065567] ? dma_buf_vmap+0x224/0x430
> [ 157.069446] ? __warn.cold+0x58/0xe4
> [ 157.073061] ? dma_buf_vmap+0x224/0x430
> [ 157.077111] ? report_bug+0x1dd/0x390
> [ 157.080842] ? handle_bug+0x5e/0xa0
> [ 157.084389] ? exc_invalid_op+0x14/0x50
> [ 157.088291] ? asm_exc_invalid_op+0x16/0x20
> [ 157.092548] ? dma_buf_vmap+0x224/0x430
> [ 157.096663] ? dma_resv_get_singleton+0x6d/0x230
> [ 157.101341] ? __pfx_dma_buf_vmap+0x10/0x10
> [ 157.105588] ? __pfx_dma_resv_get_singleton+0x10/0x10
> [ 157.110697] drm_gem_shmem_vmap+0x74/0x710
> [ 157.114866] drm_gem_vmap+0xa9/0x1b0
> [ 157.118763] drm_gem_vmap_unlocked+0x46/0xa0
> [ 157.123086] drm_gem_fb_vmap+0xab/0x300
> [ 157.126979] drm_atomic_helper_prepare_planes.part.0+0x487/0xb10
> [ 157.133032] ? lockdep_init_map_type+0x19d/0x880
> [ 157.137701] drm_atomic_helper_commit+0x13d/0x2e0
> [ 157.142671] ? drm_atomic_nonblocking_commit+0xa0/0x180
> [ 157.147988] drm_mode_atomic_ioctl+0x766/0xe40
> [...]
> [ 157.346424] ---[ end trace 0000000000000000 ]---
>
> Acquiring GEM handles for the framebuffer's GEM buffer objects prevents
> this from happening. The framebuffer's cleanup later puts the handle
> references.
>
> The Fixes tag points to commit 1a148af06000 ("drm/gem-shmem: Use dma_buf
> from GEM object instance"), which triggers the segmentation fault. The
> issue has been present before.
>
> Suggested-by: Christian König <christian.koenig at amd.com>
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> Fixes: 1a148af06000 ("drm/gem-shmem: Use dma_buf from GEM object instance")
You might want to drop this. Fixes tags are often used as limitation, e.g. patches are not backported when the patch mentioned in the Fixes tag is not present in the branch.
But in this case here it is probably a good idea to backport this anyway since the issue was always present.
> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> Cc: Anusha Srivatsa <asrivats at redhat.com>
> Cc: Christian König <christian.koenig at amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Maxime Ripard <mripard at kernel.org>
> Cc: Sumit Semwal <sumit.semwal at linaro.org>
> Cc: "Christian König" <christian.koenig at amd.com>
> Cc: linux-media at vger.kernel.org
> Cc: dri-devel at lists.freedesktop.org
> Cc: linaro-mm-sig at lists.linaro.org
> Cc: <stable at vger.kernel.org>
> ---
> drivers/gpu/drm/drm_gem.c | 44 ++++++++++++++++++--
> drivers/gpu/drm/drm_gem_framebuffer_helper.c | 7 +++-
> drivers/gpu/drm/drm_internal.h | 2 +
> 3 files changed, 48 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 19d50d254fe6..8be50b3cc9c2 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -213,6 +213,35 @@ void drm_gem_private_object_fini(struct drm_gem_object *obj)
> }
> EXPORT_SYMBOL(drm_gem_private_object_fini);
>
> +static void drm_gem_object_handle_get(struct drm_gem_object *obj)
> +{
> + struct drm_device *dev = obj->dev;
> +
> + drm_WARN_ON(dev, !mutex_is_locked(&dev->object_name_lock));
> +
> + if (obj->handle_count++ == 0)
> + drm_gem_object_get(obj);
> +}
> +
> +/**
> + * drm_gem_object_handle_get_unlocked - acquire reference on user-space handles
> + * @obj: GEM object
> + *
> + * Acquires a reference on the GEM buffer object's handle. Required
> + * to keep the GEM object alive. Call drm_gem_object_handle_put_unlocked()
> + * to release the reference.
> + */
> +void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj)
> +{
> + struct drm_device *dev = obj->dev;
> +
> + guard(mutex)(&dev->object_name_lock);
> +
> + drm_WARN_ON(dev, !obj->handle_count); // first ref taken in create-tail helper
// style comment in kernel code?
> + drm_gem_object_handle_get(obj);
> +}
> +EXPORT_SYMBOL(drm_gem_object_handle_get_unlocked);
> +
> /**
> * drm_gem_object_handle_free - release resources bound to userspace handles
> * @obj: GEM object to clean up.
> @@ -243,8 +272,14 @@ static void drm_gem_object_exported_dma_buf_free(struct drm_gem_object *obj)
> }
> }
>
> -static void
> -drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj)
> +/**
> + * drm_gem_object_handle_put_unlocked - releases reference on user-space handles
> + * @obj: GEM object
> + *
> + * Releases a reference on the GEM buffer object's handle. Possibly releases
> + * the GEM buffer object and associated dma-buf objects.
> + */
> +void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj)
> {
> struct drm_device *dev = obj->dev;
> bool final = false;
> @@ -269,6 +304,7 @@ drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj)
> if (final)
> drm_gem_object_put(obj);
> }
> +EXPORT_SYMBOL(drm_gem_object_handle_put_unlocked);
>
> /*
> * Called at device or object close to release the file's
> @@ -390,8 +426,8 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
> int ret;
>
> WARN_ON(!mutex_is_locked(&dev->object_name_lock));
> - if (obj->handle_count++ == 0)
> - drm_gem_object_get(obj);
> +
> + drm_gem_object_handle_get(obj);
>
> /*
> * Get the user-visible handle using idr. Preload and perform
> diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> index 618ce725cd75..723f1d652c01 100644
> --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> @@ -99,8 +99,10 @@ void drm_gem_fb_destroy(struct drm_framebuffer *fb)
> {
> unsigned int i;
>
> - for (i = 0; i < fb->format->num_planes; i++)
> + for (i = 0; i < fb->format->num_planes; i++) {
> + drm_gem_object_handle_put_unlocked(fb->obj[i]);
> drm_gem_object_put(fb->obj[i]);
Keeping the handle reference should be sufficient, since that should automatically keep the GEM reference as well.
Apart from that looks good to me and closes a very well known issue.
Thanks,
Christian.
> + }
>
> drm_framebuffer_cleanup(fb);
> kfree(fb);
> @@ -185,6 +187,7 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
> ret = -ENOENT;
> goto err_gem_object_put;
> }
> + drm_gem_object_handle_get_unlocked(objs[i]);
>
> min_size = (height - 1) * mode_cmd->pitches[i]
> + drm_format_info_min_pitch(info, i, width)
> @@ -195,6 +198,7 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
> "GEM object size (%zu) smaller than minimum size (%u) for plane %d\n",
> objs[i]->size, min_size, i);
> drm_gem_object_put(objs[i]);
> + drm_gem_object_handle_put_unlocked(objs[i]);
> ret = -EINVAL;
> goto err_gem_object_put;
> }
> @@ -210,6 +214,7 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
> while (i > 0) {
> --i;
> drm_gem_object_put(objs[i]);
> + drm_gem_object_handle_put_unlocked(objs[i]);
> }
> return ret;
> }
> diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h
> index 442eb31351dd..f7b414a813ae 100644
> --- a/drivers/gpu/drm/drm_internal.h
> +++ b/drivers/gpu/drm/drm_internal.h
> @@ -161,6 +161,8 @@ void drm_sysfs_lease_event(struct drm_device *dev);
>
> /* drm_gem.c */
> int drm_gem_init(struct drm_device *dev);
> +void drm_gem_object_handle_get_unlocked(struct drm_gem_object *obj);
> +void drm_gem_object_handle_put_unlocked(struct drm_gem_object *obj);
> int drm_gem_handle_create_tail(struct drm_file *file_priv,
> struct drm_gem_object *obj,
> u32 *handlep);
More information about the dri-devel
mailing list