Re: [PATCH] drm/prime: fix drm_prime_add_buf_handle

["Christian König" <christian.koenig@amd.com>]

On 6/16/25 12:38, Simona Vetter wrote:
>> 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,

No, exactly that is not guaranteed.

The previous call to HANDLE2FD stored 10 into the lookup cache additionally to 8 with the same dma_buf pointer.

And if you now get 8 or 10 as return from FD2HANDLE depends on how the red/black tree is balanced. It can be that 8 comes first or it can be that 10 comes first because the tree is only sorted by dma_buf pointer and that criteria is identical for both 8 and 10.

As far as I can see this case is not correctly handled.

Regards,
Christian.

 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.