Re: [PATCH 6.15 085/192] drm/gem: Acquire references on GEM handles for framebuffers

[Simona Vetter <simona.vetter@ffwll.ch>]

On Tue, Jul 15, 2025 at 03:43:08PM +0200, Christian König wrote:
> We are about to revert this patch. Not sure if backporting it makes sense at the moment.

I think it still makes sense, at least as an interim fix.

What I discussed with Thomas is that first we want to revert back from
gem_bo->dma_buf to gem_bo->import_attach.dmabuf everywhere. And those
patches are sprinkled over various branches/trees/releases. So it'll take
a while to catch them all. Meanwhile these two patches at least take care
of the worst fallout, they're already tested and in the pipeline - I don't
think it makes sense to hold them up and wait.

It might take us until 6.17-rc1 until we've caught all the trees and made
sure the backports of those reverts have happened, and I don't think we
should wait that long.

Cheers, Sima

> Regards,
> Christian.
> 
> On 15.07.25 15:13, Greg Kroah-Hartman wrote:
> > 6.15-stable review patch.  If anyone has any objections, please let me know.
> > 
> > ------------------
> > 
> > From: Thomas Zimmermann <tzimmermann@suse.de>
> > 
> > commit 5307dce878d4126e1b375587318955bd019c3741 upstream.
> > 
> > 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.
> > 
> > Commit 1a148af06000 ("drm/gem-shmem: Use dma_buf from GEM object
> > instance") triggers the segmentation fault easily by using the dma-buf
> > field more widely. The underlying issue with reference counting has
> > been present before.
> > 
> > v2:
> > - acquire the handle instead of the BO (Christian)
> > - fix comment style (Christian)
> > - drop the Fixes tag (Christian)
> > - rename err_ gotos
> > - add missing Link tag
> > 
> > Suggested-by: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > Link: https://elixir.bootlin.com/linux/v6.15/source/drivers/gpu/drm/drm_gem.c#L241 # [1]
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Anusha Srivatsa <asrivats@redhat.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: linux-media@vger.kernel.org
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linaro-mm-sig@lists.linaro.org
> > Cc: <stable@vger.kernel.org>
> > Reviewed-by: Christian König <christian.koenig@amd.com>
> > Link: https://lore.kernel.org/r/20250630084001.293053-1-tzimmermann@suse.de
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/gpu/drm/drm_gem.c                    |   44 ++++++++++++++++++++++++---
> >  drivers/gpu/drm/drm_gem_framebuffer_helper.c |   16 +++++----
> >  drivers/gpu/drm/drm_internal.h               |    2 +
> >  3 files changed, 51 insertions(+), 11 deletions(-)
> > 
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -212,6 +212,35 @@ void drm_gem_private_object_fini(struct
> >  }
> >  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 */
> > +	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.
> > @@ -242,8 +271,14 @@ static void drm_gem_object_exported_dma_
> >  	}
> >  }
> >  
> > -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;
> > @@ -268,6 +303,7 @@ drm_gem_object_handle_put_unlocked(struc
> >  	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
> > @@ -389,8 +425,8 @@ drm_gem_handle_create_tail(struct drm_fi
> >  	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
> > --- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
> > @@ -99,7 +99,7 @@ void drm_gem_fb_destroy(struct drm_frame
> >  	unsigned int i;
> >  
> >  	for (i = 0; i < fb->format->num_planes; i++)
> > -		drm_gem_object_put(fb->obj[i]);
> > +		drm_gem_object_handle_put_unlocked(fb->obj[i]);
> >  
> >  	drm_framebuffer_cleanup(fb);
> >  	kfree(fb);
> > @@ -182,8 +182,10 @@ int drm_gem_fb_init_with_funcs(struct dr
> >  		if (!objs[i]) {
> >  			drm_dbg_kms(dev, "Failed to lookup GEM object\n");
> >  			ret = -ENOENT;
> > -			goto err_gem_object_put;
> > +			goto err_gem_object_handle_put_unlocked;
> >  		}
> > +		drm_gem_object_handle_get_unlocked(objs[i]);
> > +		drm_gem_object_put(objs[i]);
> >  
> >  		min_size = (height - 1) * mode_cmd->pitches[i]
> >  			 + drm_format_info_min_pitch(info, i, width)
> > @@ -193,22 +195,22 @@ int drm_gem_fb_init_with_funcs(struct dr
> >  			drm_dbg_kms(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;
> > +			goto err_gem_object_handle_put_unlocked;
> >  		}
> >  	}
> >  
> >  	ret = drm_gem_fb_init(dev, fb, mode_cmd, objs, i, funcs);
> >  	if (ret)
> > -		goto err_gem_object_put;
> > +		goto err_gem_object_handle_put_unlocked;
> >  
> >  	return 0;
> >  
> > -err_gem_object_put:
> > +err_gem_object_handle_put_unlocked:
> >  	while (i > 0) {
> >  		--i;
> > -		drm_gem_object_put(objs[i]);
> > +		drm_gem_object_handle_put_unlocked(objs[i]);
> >  	}
> >  	return ret;
> >  }
> > --- 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_de
> >  
> >  /* 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);
> > 
> > 
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch