[PATCH] drm/prime: fix drm_prime_add_buf_handle

[Christian König]

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

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