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.koe...@amd.com>
> Signed-off-by: Thomas Zimmermann <tzimmerm...@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 <tzimmerm...@suse.de>
> Cc: Anusha Srivatsa <asriv...@redhat.com>
> Cc: Christian König <christian.koe...@amd.com>
> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Cc: Maxime Ripard <mrip...@kernel.org>
> Cc: Sumit Semwal <sumit.sem...@linaro.org>
> Cc: "Christian König" <christian.koe...@amd.com>
> Cc: linux-me...@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linaro-mm-...@lists.linaro.org
> Cc: <sta...@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);
