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