[PATCH] drm/prime: fix drm_prime_add_buf_handle

[Simona Vetter]

On Fri, Jun 13, 2025 at 04:10:22PM +0200, Simona Vetter wrote:
> On Fri, Jun 13, 2025 at 04:04:46PM +0200, Simona Vetter wrote:
> > On Fri, Jun 13, 2025 at 03:12:01PM +0200, Christian König wrote:
> > > It is possible through flink or IOCTLs like MODE_GETFB2 to create
> > > multiple handles for the same underlying GEM object.
> > > 
> > > But in prime we explicitely don't want to have multiple handles for the
> > > same DMA-buf. So just ignore it if a DMA-buf is exported with another
> > > handle.
> > > 
> > > This was made obvious by removing the extra check in
> > > drm_gem_prime_handle_to_dmabuf() to not add the handle if we could already
> > > find it in the housekeeping structures.
> > > 
> > > Signed-off-by: Christian König <christian.koenig at amd.com>
> > > ---
> > >  drivers/gpu/drm/drm_prime.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > > index 1d93b44c00c4..f5f30d947b61 100644
> > > --- a/drivers/gpu/drm/drm_prime.c
> > > +++ b/drivers/gpu/drm/drm_prime.c
> > > @@ -113,6 +113,17 @@ static int drm_prime_add_buf_handle(struct drm_prime_file_private *prime_fpriv,
> > >  
> > >  		rb = *p;
> > >  		pos = rb_entry(rb, struct drm_prime_member, dmabuf_rb);
> > > +
> > > +		/*
> > > +		 * Just ignore the new handle if we already have an handle for
> > > +		 * this DMA-buf.
> > > +		 */
> > > +		if (dma_buf == pos->dma_buf) {
> > > +			dma_buf_put(dma_buf);
> > > +			kfree(member);
> > > +			return 0;
> > 
> > This feels a bit brittle, because this case should only be possible when
> > called from drm_gem_prime_handle_to_dmabuf and not from
> > drm_gem_prime_fd_to_handle() (where it would indicate a real race and
> > hence bug in our code).
> > 
> > I think  drm_gem_prime_fd_to_handle() should WARN_ON if it hits this case. 
> 
> Simplest would be to return -EEXISTS here and then either silence that
> errno or warn about it in the two call sites. Not pretty, but everything
> else looks worse.

Did you send a v2 for this one? I think we should at least sort out the
regression and then figure out the longer-standing issue. Not even sure
that's a regression from the r-b tree conversion or whether that goes back
to my original linke-list walk code.
-Sima

> > Otherwise yes this is the functional change that I've missed :-/ Note that
> > there's no race in the original code, because it's all protected by the
> > file_priv->prime.lock. Which means I think you're claim that you've only
> > widened the race with your patch is wrong.
> > 
> > Cheers, Sima
> > 
> > > +
> > > +		}
> > >  		if (dma_buf > pos->dma_buf)
> > >  			p = &rb->rb_right;
> > >  		else
> > > -- 
> > > 2.34.1
> > > 
> > 
> > -- 
> > Simona Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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