[PATCH] drm/prime: fix drm_prime_add_buf_handle

[Simona Vetter]

On Tue, 17 Jun 2025 at 16:11, Simona Vetter <simona.vetter at 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 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.
> >
> > 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