[PATCH] drm/prime: fix drm_prime_add_buf_handle

[Christian König]

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? 

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.

Regards,
Christian.

> 
> Cheers, Sima
> 
>> +
>> +		}
>>  		if (dma_buf > pos->dma_buf)
>>  			p = &rb->rb_right;
>>  		else
>> -- 
>> 2.34.1
>>
>