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 <[email protected]> > > > --- > > > 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
