Trying to fix drm/prime: remove drm_prime_lookup_buf_by_handle

Trying to fix drm/prime: remove drm_prime_lookup_buf_by_handle

[Christian König]

The problem with this patch is that we now don't check if the handle
already existed any more, but just blindly try to add it again.

This obviously breaks re-exporting DMA-bufs but also surfaces an issue
that we don't seem to take into account that multiple handles can point
to the same GEM object.

Try to fix this by just ignoring adding the same DMA-buf to the rb tree
multiple times. Not sure if that is the best approach, but it should fix
the issue at hand.

Regards,
Christian.

[PATCH] drm/prime: fix drm_prime_add_buf_handle

[Christian König]

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@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;
+
+		}
 		if (dma_buf > pos->dma_buf)
 			p = &rb->rb_right;
 		else
-- 
2.34.1

Re: [PATCH] drm/prime: fix drm_prime_add_buf_handle

[Simona Vetter]

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

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

Re: [PATCH] drm/prime: fix drm_prime_add_buf_handle

[Simona Vetter]

On Fri, Jun 13, 2025 at 04:04:46PM +0200, 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@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. 

Simplest would be to return -EEXISTS here and then either silence that
errno or warn about it in the two call sites. Not pretty, but everything
else looks worse.
-Sima

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

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

Re: [PATCH] drm/prime: fix drm_prime_add_buf_handle

[Simona Vetter]

On Fri, Jun 13, 2025 at 04:10:22PM +0200, Simona Vetter wrote:
> On Fri, Jun 13, 2025 at 04:04:46PM +0200, 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@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. 
> 
> Simplest would be to return -EEXISTS here and then either silence that
> errno or warn about it in the two call sites. Not pretty, but everything
> else looks worse.

Did you send a v2 for this one? I think we should at least sort out the
regression and then figure out the longer-standing issue. Not even sure
that's a regression from the r-b tree conversion or whether that goes back
to my original linke-list walk code.
-Sima

> > 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.
> > 
> > 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
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH] drm/prime: fix drm_prime_add_buf_handle

[Simona Vetter]

On Tue, 17 Jun 2025 at 16:11, Simona Vetter <simona.vetter@ffwll.ch> wrote:
>
> On Fri, Jun 13, 2025 at 04:10:22PM +0200, Simona Vetter wrote:
> > On Fri, Jun 13, 2025 at 04:04:46PM +0200, 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@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.
> >
> > Simplest would be to return -EEXISTS here and then either silence that
> > errno or warn about it in the two call sites. Not pretty, but everything
> > else looks worse.
>
> Did you send a v2 for this one? I think we should at least sort out the
> regression and then figure out the longer-standing issue. Not even sure
> that's a regression from the r-b tree conversion or whether that goes back
> to my original linke-list walk code.

Yeah I think flink or any of the other buffer handle duplication
tricks just mess things up in funny ways. But as long as userspace
doesn't do those, it should be all fine I think ...
-Sima

> > > 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.
> > >
> > > 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
> >
> > --
> > Simona Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
>
> --
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



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

Re: [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@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
>>
> 

Re: [PATCH] drm/prime: fix drm_prime_add_buf_handle

[Simona Vetter]

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

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

Re: [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@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
>>>>
>>>
>>
> 

Re: [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@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

Re: [PATCH] drm/prime: fix drm_prime_add_buf_handle

[Christian König]

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.

Re: [PATCH] drm/prime: fix drm_prime_add_buf_handle

[Simona Vetter]

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

RE: Trying to fix drm/prime: remove drm_prime_lookup_buf_by_handle

[Saarinen, Jani]

Hi, 
> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Friday, 13 June 2025 16.12
> To: Saarinen, Jani <jani.saarinen@intel.com>; jani.nikula@linux.intel.com;
> tursulin@ursulin.net; simona.vetter@ffwll.ch; tzimmermann@suse.de; dri-
> devel@lists.freedesktop.org
> Subject: Trying to fix drm/prime: remove drm_prime_lookup_buf_by_handle
> 
> The problem with this patch is that we now don't check if the handle already
> existed any more, but just blindly try to add it again.
> 
> This obviously breaks re-exporting DMA-bufs but also surfaces an issue that
> we don't seem to take into account that multiple handles can point to the
> same GEM object.
> 
> Try to fix this by just ignoring adding the same DMA-buf to the rb tree multiple
> times. Not sure if that is the best approach, but it should fix the issue at hand.

Please send also to intel gfx and xe mailing lists to see how that works as now both as as discussed broken 
I915: https://intel-gfx-ci.01.org/tree/drm-tip/index.html?testfilter=prime 
Xe: https://intel-gfx-ci.01.org/tree/intel-xe/index.html?testfilter=prime

> 
> Regards,
> Christian.
>