[PATCH] drm/prime: fix drm_prime_add_buf_handle

[Simona Vetter]

On Fri, Jun 13, 2025 at 04:12:47PM +0200, Christian König wrote:
> On 6/13/25 16:04, 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. 
> > 
> > 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.
> 
> Yeah, agree. I'm always confused that there are two locks to protect the data structures.
> 
> But there is indeed a problem in the existing code. What happens if a
> GEM handle duplicate is exported with drm_prime_add_buf_handle()? E.g.
> something created by GETFB2? 

The uniqueness guarantee only extends to FB2HANDLE, because that's the
case userspace cannot figure out any other way. For flink import you can
compare the flink name (those are global), and for other ioctl like
GETFB(2) you just always get a new name that you need to close() yourself.

I guess if you want a unique name for these others you could do a
rount-trip through a dma-buf :-P

But the reaons dma-buf import was special was that before we had a real
inode or the KMP syscall there was just no way to compare dma-buf for
identity, and so we needed a special guarantee. Probably the funniest
piece of uapi we have :-/

> IIRC AMD once had a test case which exercised exactly that. I'm not 100%
> sure what would happen here, but it looks not correct to me.

Yeah I think the real-world GETFB are only for when you know it's not one
of your own buffers, so all fine. Or we haven't tested this stuff enough
yet ... Either way, userpace can fix it with a round-trip through
FD2HANDLE.

Cheers, Sima

> 
> Regards,
> Christian.
> 
> > 
> > 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