[PATCH] drm/prime: fix drm_prime_add_buf_handle

[Simona Vetter]

On Fri, Jun 13, 2025 at 05:03:39PM +0200, Christian König wrote:
> On 6/13/25 16:35, Simona Vetter wrote:
> > 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.
> 
> Well that sounds like you didn't understood what I meant.
> 
> The problem here is that we mess up FD2HANDLE if I'm not completely mistaken.
> 
> > 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

I guess I should have elaborated what I mean here with this off-hand
remark, see below.

> I advised that before as well, but exactly that's what is not working as far as I can see.
> 
> Let's go over this example:
> 1. We have GEM handle 8.
> 2. Export GEM handle 8 as DMA-buf and get an FD.
> 3. Import the DMA-buf FD again with FD2HANDLE and get 8.
> 4. Now 8 is used in a FB config.
> 5. Somebody calls GETFB2 and gets 10 instead 8 for the same BO.
> 
> 6. Now FD2HANDLE is called with 10 and here is what happens:
> 
> 	drm_prime_lookup_buf_by_handle() is called for handle 10, so we
> 	don't find anything.
> 
> 	obj->dma_buf is true so we branch into the if and call
> 	drm_prime_add_buf_handle() with handle 10.
> 
> Now we have called drm_prime_add_buf_handle() both for handle 8 and
> handle 10 and so we have both 8 and 10 for the same DMA-buf in our tree.

So this is the case that broke, and which the various igt prime tests
actually had testcases for. Unless I'm completely confused here now.

> So FD2HANDLE could return either 8 or 10 depending on which is looked up
> first.
> 
> I'm not 100% sure if that has any bad consequences, but I'm pretty sure
> that this is not intentional.
> 
> Should we fix that? If yes than how?

I dont think there's an issue, all we guarantee is that if you call
FD2HANDLE or HANDLE2FD, then you get something consistent back. From a
userspace pov there's two cases:

1. We've already seen this buffer, it got handle 8, that's the one we've
stored in the lookup caches. If you then do GETFB2 you get handle 10,
which could be confusing. So you do

	temp_dmabuf_fd = ioctl(HANDLE2FD, 10);
	new_id = ioctl(FD2HANDLE, temp_dmabuf_fd);
	close(temp_dma_buf_fd);
	ioctl(GEM_CLOSE, 10);

At this point new_id is 8, and you already have that in your userspace
cache, so all is good.

2. Userspace doesn't have the buffer already, but it doesn't know that. It
does the exact dance as above, except this time around it gets back the
same gem_handle as it got from GETFB2 and knows that it does not have to
close that handle (since it's the only one), and that it should add that
handle to the userspace-side dma-buf import/export side.

It's a bit a contrived dance, but I don't think we have an issue here.

Cheers, Sima

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