Re: [PATCH] drm/prime: fix drm_prime_add_buf_handle

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

On Mon, Jun 16, 2025 at 01:38:17PM +0200, Christian König wrote:
> 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.

Hm right, I did type some testcases with flink, but not one that does
funny stuff like this :-/
-Sima

> 
> 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.

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