| Message ID | 20220803083237.3701-1-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/gem: Fix GEM handle release errors | expand |
Am 03.08.22 um 10:32 schrieb Jeffy Chen: > Currently we are assuming a one to one mapping between dmabuf and handle > when releasing GEM handles. > > But that is not always true, since we would create extra handles for the > GEM obj in cases like gem_open() and getfb{,2}(). > > A similar issue was reported at: > https://lore.kernel.org/all/20211105083308.392156-1-jay.xu@rock-chips.com/ > > Another problem is that the drm_gem_remove_prime_handles() now only > remove handle to the exported dmabuf (gem_obj->dma_buf), so the imported > ones would leak: > WARNING: CPU: 2 PID: 236 at drivers/gpu/drm/drm_prime.c:228 drm_prime_destroy_file_private+0x18/0x24 > > Let's fix these by using handle to find the exact map to remove. Well we are clearly something missing here. As far as I can see the current code is correct. Creating multiple GEM handles for the same DMA-buf is possible, but illegal. In other words when a GEM handle is exported as DMA-buf and imported again you should intentionally always get the same handle. So this is pretty much a clear NAK to this patch since it shouldn't be necessary or something is seriously broken somewhere else. Regards, Christian. > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > --- > > Changes in v2: > Fix a typo of rbtree. > > drivers/gpu/drm/drm_gem.c | 17 +---------------- > drivers/gpu/drm/drm_internal.h | 4 ++-- > drivers/gpu/drm/drm_prime.c | 20 ++++++++++++-------- > 3 files changed, 15 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index eb0c2d041f13..ed39da383570 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -168,21 +168,6 @@ void drm_gem_private_object_init(struct drm_device *dev, > } > EXPORT_SYMBOL(drm_gem_private_object_init); > > -static void > -drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) > -{ > - /* > - * Note: obj->dma_buf can't disappear as long as we still hold a > - * handle reference in obj->handle_count. > - */ > - mutex_lock(&filp->prime.lock); > - if (obj->dma_buf) { > - drm_prime_remove_buf_handle_locked(&filp->prime, > - obj->dma_buf); > - } > - mutex_unlock(&filp->prime.lock); > -} > - > /** > * drm_gem_object_handle_free - release resources bound to userspace handles > * @obj: GEM object to clean up. > @@ -253,7 +238,7 @@ drm_gem_object_release_handle(int id, void *ptr, void *data) > if (obj->funcs->close) > obj->funcs->close(obj, file_priv); > > - drm_gem_remove_prime_handles(obj, file_priv); > + drm_prime_remove_buf_handle(&file_priv->prime, id); > drm_vma_node_revoke(&obj->vma_node, file_priv); > > drm_gem_object_handle_put_unlocked(obj); > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index 1fbbc19f1ac0..7bb98e6a446d 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -74,8 +74,8 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, > > void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv); > void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv); > -void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv, > - struct dma_buf *dma_buf); > +void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, > + uint32_t handle); > > /* drm_drv.c */ > struct drm_minor *drm_minor_acquire(unsigned int minor_id); > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c > index e3f09f18110c..bd5366b16381 100644 > --- a/drivers/gpu/drm/drm_prime.c > +++ b/drivers/gpu/drm/drm_prime.c > @@ -190,29 +190,33 @@ static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpri > return -ENOENT; > } > > -void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv, > - struct dma_buf *dma_buf) > +void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, > + uint32_t handle) > { > struct rb_node *rb; > > - rb = prime_fpriv->dmabufs.rb_node; > + mutex_lock(&prime_fpriv->lock); > + > + rb = prime_fpriv->handles.rb_node; > while (rb) { > struct drm_prime_member *member; > > - member = rb_entry(rb, struct drm_prime_member, dmabuf_rb); > - if (member->dma_buf == dma_buf) { > + member = rb_entry(rb, struct drm_prime_member, handle_rb); > + if (member->handle == handle) { > rb_erase(&member->handle_rb, &prime_fpriv->handles); > rb_erase(&member->dmabuf_rb, &prime_fpriv->dmabufs); > > - dma_buf_put(dma_buf); > + dma_buf_put(member->dma_buf); > kfree(member); > - return; > - } else if (member->dma_buf < dma_buf) { > + break; > + } else if (member->handle < handle) { > rb = rb->rb_right; > } else { > rb = rb->rb_left; > } > } > + > + mutex_unlock(&prime_fpriv->lock); > } > > void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv)
Hi Christian, Thanks for your reply, and sorry i didn't make it clear. On 8/8 星期一 0:52, Christian König wrote: > Am 03.08.22 um 10:32 schrieb Jeffy Chen: >> Currently we are assuming a one to one mapping between dmabuf and handle >> when releasing GEM handles. >> >> But that is not always true, since we would create extra handles for the >> GEM obj in cases like gem_open() and getfb{,2}(). >> >> A similar issue was reported at: >> https://lore.kernel.org/all/20211105083308.392156-1-jay.xu@rock-chips.com/ >> >> Another problem is that the drm_gem_remove_prime_handles() now only >> remove handle to the exported dmabuf (gem_obj->dma_buf), so the imported >> ones would leak: >> WARNING: CPU: 2 PID: 236 at drivers/gpu/drm/drm_prime.c:228 >> drm_prime_destroy_file_private+0x18/0x24 >> >> Let's fix these by using handle to find the exact map to remove. > > Well we are clearly something missing here. As far as I can see the > current code is correct. > > Creating multiple GEM handles for the same DMA-buf is possible, but > illegal. > > In other words when a GEM handle is exported as DMA-buf and imported > again you should intentionally always get the same handle. These issue are not about having handles for importing an exported dma-buf case, but for having multiple handles to a GEM object(which means having multiple handles to a dma-buf). I know the drm-prime is trying to make dma-buf and handle maps one to one, but the drm-gem is allowing to create extra handles for a GEM object, for example: drm_gem_open_ioctl -> drm_gem_handle_create_tail drm_mode_getfb2_ioctl -> drm_gem_handle_create drm_mode_getfb -> fb->funcs->create_handle So we are allowing GEM object to have multiple handles, and GEM object could have at most one dma-buf, doesn't that means that dma-buf could map to multiple handles? Or should we rewrite the GEM framework to limit GEM object with uniq handle? The other issue is that we are leaking dma-buf <-> handle map for the imported dma-buf, since the drm_gem_remove_prime_handles doesn't take care of obj->import_attach->dmabuf. But of cause this can be fixed in other way: +++ b/drivers/gpu/drm/drm_gem.c @@ -180,6 +180,9 @@ drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) drm_prime_remove_buf_handle_locked(&filp->prime, obj->dma_buf); } + if (obj->import_attach) + drm_prime_remove_buf_handle_locked(&filp->prime, + obj->import_attach->dmabuf); mutex_unlock(&filp->prime.lock); } > So this is pretty much a clear NAK to this patch since it shouldn't be > necessary or something is seriously broken somewhere else. > > Regards, > Christian. > >> >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >> --- >> >> Changes in v2: >> Fix a typo of rbtree. >> >> drivers/gpu/drm/drm_gem.c | 17 +---------------- >> drivers/gpu/drm/drm_internal.h | 4 ++-- >> drivers/gpu/drm/drm_prime.c | 20 ++++++++++++-------- >> 3 files changed, 15 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >> index eb0c2d041f13..ed39da383570 100644 >> --- a/drivers/gpu/drm/drm_gem.c >> +++ b/drivers/gpu/drm/drm_gem.c >> @@ -168,21 +168,6 @@ void drm_gem_private_object_init(struct >> drm_device *dev, >> } >> EXPORT_SYMBOL(drm_gem_private_object_init); >> -static void >> -drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct >> drm_file *filp) >> -{ >> - /* >> - * Note: obj->dma_buf can't disappear as long as we still hold a >> - * handle reference in obj->handle_count. >> - */ >> - mutex_lock(&filp->prime.lock); >> - if (obj->dma_buf) { >> - drm_prime_remove_buf_handle_locked(&filp->prime, >> - obj->dma_buf); >> - } >> - mutex_unlock(&filp->prime.lock); >> -} >> - >> /** >> * drm_gem_object_handle_free - release resources bound to userspace >> handles >> * @obj: GEM object to clean up. >> @@ -253,7 +238,7 @@ drm_gem_object_release_handle(int id, void *ptr, >> void *data) >> if (obj->funcs->close) >> obj->funcs->close(obj, file_priv); >> - drm_gem_remove_prime_handles(obj, file_priv); >> + drm_prime_remove_buf_handle(&file_priv->prime, id); >> drm_vma_node_revoke(&obj->vma_node, file_priv); >> drm_gem_object_handle_put_unlocked(obj); >> diff --git a/drivers/gpu/drm/drm_internal.h >> b/drivers/gpu/drm/drm_internal.h >> index 1fbbc19f1ac0..7bb98e6a446d 100644 >> --- a/drivers/gpu/drm/drm_internal.h >> +++ b/drivers/gpu/drm/drm_internal.h >> @@ -74,8 +74,8 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device >> *dev, void *data, >> void drm_prime_init_file_private(struct drm_prime_file_private >> *prime_fpriv); >> void drm_prime_destroy_file_private(struct drm_prime_file_private >> *prime_fpriv); >> -void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private >> *prime_fpriv, >> - struct dma_buf *dma_buf); >> +void drm_prime_remove_buf_handle(struct drm_prime_file_private >> *prime_fpriv, >> + uint32_t handle); >> /* drm_drv.c */ >> struct drm_minor *drm_minor_acquire(unsigned int minor_id); >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >> index e3f09f18110c..bd5366b16381 100644 >> --- a/drivers/gpu/drm/drm_prime.c >> +++ b/drivers/gpu/drm/drm_prime.c >> @@ -190,29 +190,33 @@ static int drm_prime_lookup_buf_handle(struct >> drm_prime_file_private *prime_fpri >> return -ENOENT; >> } >> -void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private >> *prime_fpriv, >> - struct dma_buf *dma_buf) >> +void drm_prime_remove_buf_handle(struct drm_prime_file_private >> *prime_fpriv, >> + uint32_t handle) >> { >> struct rb_node *rb; >> - rb = prime_fpriv->dmabufs.rb_node; >> + mutex_lock(&prime_fpriv->lock); >> + >> + rb = prime_fpriv->handles.rb_node; >> while (rb) { >> struct drm_prime_member *member; >> - member = rb_entry(rb, struct drm_prime_member, dmabuf_rb); >> - if (member->dma_buf == dma_buf) { >> + member = rb_entry(rb, struct drm_prime_member, handle_rb); >> + if (member->handle == handle) { >> rb_erase(&member->handle_rb, &prime_fpriv->handles); >> rb_erase(&member->dmabuf_rb, &prime_fpriv->dmabufs); >> - dma_buf_put(dma_buf); >> + dma_buf_put(member->dma_buf); >> kfree(member); >> - return; >> - } else if (member->dma_buf < dma_buf) { >> + break; >> + } else if (member->handle < handle) { >> rb = rb->rb_right; >> } else { >> rb = rb->rb_left; >> } >> } >> + >> + mutex_unlock(&prime_fpriv->lock); >> } >> void drm_prime_init_file_private(struct drm_prime_file_private >> *prime_fpriv) > >
Hi Jeffy, Am 08.08.22 um 05:51 schrieb Chen Jeffy: > Hi Christian, > > Thanks for your reply, and sorry i didn't make it clear. > > On 8/8 星期一 0:52, Christian König wrote: >> Am 03.08.22 um 10:32 schrieb Jeffy Chen: >>> Currently we are assuming a one to one mapping between dmabuf and >>> handle >>> when releasing GEM handles. >>> >>> But that is not always true, since we would create extra handles for >>> the >>> GEM obj in cases like gem_open() and getfb{,2}(). >>> >>> A similar issue was reported at: >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20211105083308.392156-1-jay.xu%40rock-chips.com%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7Cd7488e9f235041f7e84408da78f14882%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637955274964656400%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9G2YbHcYUs1VQYyvjXwLzYawNw%2BP8i%2BjjPBSHx3r2yg%3D&reserved=0 >>> >>> >>> Another problem is that the drm_gem_remove_prime_handles() now only >>> remove handle to the exported dmabuf (gem_obj->dma_buf), so the >>> imported >>> ones would leak: >>> WARNING: CPU: 2 PID: 236 at drivers/gpu/drm/drm_prime.c:228 >>> drm_prime_destroy_file_private+0x18/0x24 >>> >>> Let's fix these by using handle to find the exact map to remove. >> >> Well we are clearly something missing here. As far as I can see the >> current code is correct. >> >> Creating multiple GEM handles for the same DMA-buf is possible, but >> illegal. > >> In other words when a GEM handle is exported as DMA-buf and imported >> again you should intentionally always get the same handle. > > These issue are not about having handles for importing an exported > dma-buf case, but for having multiple handles to a GEM object(which > means having multiple handles to a dma-buf). > > I know the drm-prime is trying to make dma-buf and handle maps one to > one, but the drm-gem is allowing to create extra handles for a GEM > object, for example: > drm_gem_open_ioctl -> drm_gem_handle_create_tail > drm_mode_getfb2_ioctl -> drm_gem_handle_create > drm_mode_getfb -> fb->funcs->create_handle Yes, so far that's correct. > > > So we are allowing GEM object to have multiple handles, and GEM object > could have at most one dma-buf, doesn't that means that dma-buf could > map to multiple handles? No, at least not for the same GEM file private. That's the reason why the rb is indexed by the dma_buf object and not the handle. In other words the rb is so that you have exactly one handle for each dma_buf in each file private. > > Or should we rewrite the GEM framework to limit GEM object with uniq > handle? No, the extra handles are expected because when you call drm_mode_getfb*() and drm_gem_open_ioctl() the caller now owns the returned GEM handle. > > The other issue is that we are leaking dma-buf <-> handle map for the > imported dma-buf, since the drm_gem_remove_prime_handles doesn't take > care of obj->import_attach->dmabuf. No, that's correct as well. obj->dma_buf is set even for imported DMA-buf objects. See drm_gem_prime_fd_to_handle(). Regards, Christian. > > But of cause this can be fixed in other way: > +++ b/drivers/gpu/drm/drm_gem.c > @@ -180,6 +180,9 @@ drm_gem_remove_prime_handles(struct drm_gem_object > *obj, struct drm_file *filp) > drm_prime_remove_buf_handle_locked(&filp->prime, > obj->dma_buf); > } > + if (obj->import_attach) > + drm_prime_remove_buf_handle_locked(&filp->prime, > + obj->import_attach->dmabuf); > mutex_unlock(&filp->prime.lock); > } > > >> So this is pretty much a clear NAK to this patch since it shouldn't >> be necessary or something is seriously broken somewhere else. >> >> Regards, >> Christian. >> >>> >>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >>> --- >>> >>> Changes in v2: >>> Fix a typo of rbtree. >>> >>> drivers/gpu/drm/drm_gem.c | 17 +---------------- >>> drivers/gpu/drm/drm_internal.h | 4 ++-- >>> drivers/gpu/drm/drm_prime.c | 20 ++++++++++++-------- >>> 3 files changed, 15 insertions(+), 26 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >>> index eb0c2d041f13..ed39da383570 100644 >>> --- a/drivers/gpu/drm/drm_gem.c >>> +++ b/drivers/gpu/drm/drm_gem.c >>> @@ -168,21 +168,6 @@ void drm_gem_private_object_init(struct >>> drm_device *dev, >>> } >>> EXPORT_SYMBOL(drm_gem_private_object_init); >>> -static void >>> -drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct >>> drm_file *filp) >>> -{ >>> - /* >>> - * Note: obj->dma_buf can't disappear as long as we still hold a >>> - * handle reference in obj->handle_count. >>> - */ >>> - mutex_lock(&filp->prime.lock); >>> - if (obj->dma_buf) { >>> - drm_prime_remove_buf_handle_locked(&filp->prime, >>> - obj->dma_buf); >>> - } >>> - mutex_unlock(&filp->prime.lock); >>> -} >>> - >>> /** >>> * drm_gem_object_handle_free - release resources bound to >>> userspace handles >>> * @obj: GEM object to clean up. >>> @@ -253,7 +238,7 @@ drm_gem_object_release_handle(int id, void *ptr, >>> void *data) >>> if (obj->funcs->close) >>> obj->funcs->close(obj, file_priv); >>> - drm_gem_remove_prime_handles(obj, file_priv); >>> + drm_prime_remove_buf_handle(&file_priv->prime, id); >>> drm_vma_node_revoke(&obj->vma_node, file_priv); >>> drm_gem_object_handle_put_unlocked(obj); >>> diff --git a/drivers/gpu/drm/drm_internal.h >>> b/drivers/gpu/drm/drm_internal.h >>> index 1fbbc19f1ac0..7bb98e6a446d 100644 >>> --- a/drivers/gpu/drm/drm_internal.h >>> +++ b/drivers/gpu/drm/drm_internal.h >>> @@ -74,8 +74,8 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device >>> *dev, void *data, >>> void drm_prime_init_file_private(struct drm_prime_file_private >>> *prime_fpriv); >>> void drm_prime_destroy_file_private(struct drm_prime_file_private >>> *prime_fpriv); >>> -void drm_prime_remove_buf_handle_locked(struct >>> drm_prime_file_private *prime_fpriv, >>> - struct dma_buf *dma_buf); >>> +void drm_prime_remove_buf_handle(struct drm_prime_file_private >>> *prime_fpriv, >>> + uint32_t handle); >>> /* drm_drv.c */ >>> struct drm_minor *drm_minor_acquire(unsigned int minor_id); >>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>> index e3f09f18110c..bd5366b16381 100644 >>> --- a/drivers/gpu/drm/drm_prime.c >>> +++ b/drivers/gpu/drm/drm_prime.c >>> @@ -190,29 +190,33 @@ static int drm_prime_lookup_buf_handle(struct >>> drm_prime_file_private *prime_fpri >>> return -ENOENT; >>> } >>> -void drm_prime_remove_buf_handle_locked(struct >>> drm_prime_file_private *prime_fpriv, >>> - struct dma_buf *dma_buf) >>> +void drm_prime_remove_buf_handle(struct drm_prime_file_private >>> *prime_fpriv, >>> + uint32_t handle) >>> { >>> struct rb_node *rb; >>> - rb = prime_fpriv->dmabufs.rb_node; >>> + mutex_lock(&prime_fpriv->lock); >>> + >>> + rb = prime_fpriv->handles.rb_node; >>> while (rb) { >>> struct drm_prime_member *member; >>> - member = rb_entry(rb, struct drm_prime_member, dmabuf_rb); >>> - if (member->dma_buf == dma_buf) { >>> + member = rb_entry(rb, struct drm_prime_member, handle_rb); >>> + if (member->handle == handle) { >>> rb_erase(&member->handle_rb, &prime_fpriv->handles); >>> rb_erase(&member->dmabuf_rb, &prime_fpriv->dmabufs); >>> - dma_buf_put(dma_buf); >>> + dma_buf_put(member->dma_buf); >>> kfree(member); >>> - return; >>> - } else if (member->dma_buf < dma_buf) { >>> + break; >>> + } else if (member->handle < handle) { >>> rb = rb->rb_right; >>> } else { >>> rb = rb->rb_left; >>> } >>> } >>> + >>> + mutex_unlock(&prime_fpriv->lock); >>> } >>> void drm_prime_init_file_private(struct drm_prime_file_private >>> *prime_fpriv) >> >> >
Hi Christian, On 8/9 星期二 2:03, Christian König wrote: > Hi Jeffy, > > Am 08.08.22 um 05:51 schrieb Chen Jeffy: >> Hi Christian, >> >> Thanks for your reply, and sorry i didn't make it clear. >> >> On 8/8 星期一 0:52, Christian König wrote: >>> Am 03.08.22 um 10:32 schrieb Jeffy Chen: >>>> Currently we are assuming a one to one mapping between dmabuf and >>>> handle >>>> when releasing GEM handles. >>>> >>>> But that is not always true, since we would create extra handles for >>>> the >>>> GEM obj in cases like gem_open() and getfb{,2}(). >>>> >>>> A similar issue was reported at: >>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20211105083308.392156-1-jay.xu%40rock-chips.com%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7Cd7488e9f235041f7e84408da78f14882%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637955274964656400%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9G2YbHcYUs1VQYyvjXwLzYawNw%2BP8i%2BjjPBSHx3r2yg%3D&reserved=0 >>>> >>>> Another problem is that the drm_gem_remove_prime_handles() now only >>>> remove handle to the exported dmabuf (gem_obj->dma_buf), so the >>>> imported >>>> ones would leak: >>>> WARNING: CPU: 2 PID: 236 at drivers/gpu/drm/drm_prime.c:228 >>>> drm_prime_destroy_file_private+0x18/0x24 >>>> >>>> Let's fix these by using handle to find the exact map to remove. >>> >>> Well we are clearly something missing here. As far as I can see the >>> current code is correct. >>> >>> Creating multiple GEM handles for the same DMA-buf is possible, but >>> illegal. > >>> In other words when a GEM handle is exported as DMA-buf and imported >>> again you should intentionally always get the same handle. >> >> These issue are not about having handles for importing an exported >> dma-buf case, but for having multiple handles to a GEM object(which >> means having multiple handles to a dma-buf). >> >> I know the drm-prime is trying to make dma-buf and handle maps one to >> one, but the drm-gem is allowing to create extra handles for a GEM >> object, for example: >> drm_gem_open_ioctl -> drm_gem_handle_create_tail >> drm_mode_getfb2_ioctl -> drm_gem_handle_create >> drm_mode_getfb -> fb->funcs->create_handle > > Yes, so far that's correct. > >> >> >> So we are allowing GEM object to have multiple handles, and GEM object >> could have at most one dma-buf, doesn't that means that dma-buf could >> map to multiple handles? > > No, at least not for the same GEM file private. That's the reason why > the rb is indexed by the dma_buf object and not the handle. > > In other words the rb is so that you have exactly one handle for each > dma_buf in each file private. I don't think so, because if user get multiple handles for the same GEM obj and use drm_gem_prime_handle_to_fd() for those handles, the current code would try to add multiple maps to rb: drm_prime_add_buf_handle(buf_1, hdl_1) drm_prime_add_buf_handle(buf_1, hdl_2) ... drm_prime_add_buf_handle(buf_1, hdl_n) > >> >> Or should we rewrite the GEM framework to limit GEM object with uniq >> handle? > > No, the extra handles are expected because when you call > drm_mode_getfb*() and drm_gem_open_ioctl() the caller now owns the > returned GEM handle. > >> >> The other issue is that we are leaking dma-buf <-> handle map for the >> imported dma-buf, since the drm_gem_remove_prime_handles doesn't take >> care of obj->import_attach->dmabuf. > > No, that's correct as well. obj->dma_buf is set even for imported > DMA-buf objects. See drm_gem_prime_fd_to_handle(). Well, that obj->dma_buf would be set in drm_gem_prime_fd_to_handle(create new handle), and cleared when releasing the latest handle(release handle). So it doesn't cover other handle creating path. For example, a imported dma buf: drm_gem_prime_fd_to_handle <-- we got a handle and obj->dma_buf and obj->import_attach->dmabuf drm_gem_handle_delete <-- we lost that handle and obj->dma_buf cleared drm_gem_open_ioctl/or getfb* <-- we got a new handle and obj->import_attach->dmabuf drm_gem_handle_delete <-- we lost that handle and obj->dma_buf is null, which means rb leaks. > > Regards, > Christian. > >> >> But of cause this can be fixed in other way: >> +++ b/drivers/gpu/drm/drm_gem.c >> @@ -180,6 +180,9 @@ drm_gem_remove_prime_handles(struct drm_gem_object >> *obj, struct drm_file *filp) >> drm_prime_remove_buf_handle_locked(&filp->prime, >> obj->dma_buf); >> } >> + if (obj->import_attach) >> + drm_prime_remove_buf_handle_locked(&filp->prime, >> + obj->import_attach->dmabuf); >> mutex_unlock(&filp->prime.lock); >> } >> >> >>> So this is pretty much a clear NAK to this patch since it shouldn't >>> be necessary or something is seriously broken somewhere else. >>> >>> Regards, >>> Christian. >>> >>>> >>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >>>> --- >>>> >>>> Changes in v2: >>>> Fix a typo of rbtree. >>>> >>>> drivers/gpu/drm/drm_gem.c | 17 +---------------- >>>> drivers/gpu/drm/drm_internal.h | 4 ++-- >>>> drivers/gpu/drm/drm_prime.c | 20 ++++++++++++-------- >>>> 3 files changed, 15 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >>>> index eb0c2d041f13..ed39da383570 100644 >>>> --- a/drivers/gpu/drm/drm_gem.c >>>> +++ b/drivers/gpu/drm/drm_gem.c >>>> @@ -168,21 +168,6 @@ void drm_gem_private_object_init(struct >>>> drm_device *dev, >>>> } >>>> EXPORT_SYMBOL(drm_gem_private_object_init); >>>> -static void >>>> -drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct >>>> drm_file *filp) >>>> -{ >>>> - /* >>>> - * Note: obj->dma_buf can't disappear as long as we still hold a >>>> - * handle reference in obj->handle_count. >>>> - */ >>>> - mutex_lock(&filp->prime.lock); >>>> - if (obj->dma_buf) { >>>> - drm_prime_remove_buf_handle_locked(&filp->prime, >>>> - obj->dma_buf); >>>> - } >>>> - mutex_unlock(&filp->prime.lock); >>>> -} >>>> - >>>> /** >>>> * drm_gem_object_handle_free - release resources bound to >>>> userspace handles >>>> * @obj: GEM object to clean up. >>>> @@ -253,7 +238,7 @@ drm_gem_object_release_handle(int id, void *ptr, >>>> void *data) >>>> if (obj->funcs->close) >>>> obj->funcs->close(obj, file_priv); >>>> - drm_gem_remove_prime_handles(obj, file_priv); >>>> + drm_prime_remove_buf_handle(&file_priv->prime, id); >>>> drm_vma_node_revoke(&obj->vma_node, file_priv); >>>> drm_gem_object_handle_put_unlocked(obj); >>>> diff --git a/drivers/gpu/drm/drm_internal.h >>>> b/drivers/gpu/drm/drm_internal.h >>>> index 1fbbc19f1ac0..7bb98e6a446d 100644 >>>> --- a/drivers/gpu/drm/drm_internal.h >>>> +++ b/drivers/gpu/drm/drm_internal.h >>>> @@ -74,8 +74,8 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device >>>> *dev, void *data, >>>> void drm_prime_init_file_private(struct drm_prime_file_private >>>> *prime_fpriv); >>>> void drm_prime_destroy_file_private(struct drm_prime_file_private >>>> *prime_fpriv); >>>> -void drm_prime_remove_buf_handle_locked(struct >>>> drm_prime_file_private *prime_fpriv, >>>> - struct dma_buf *dma_buf); >>>> +void drm_prime_remove_buf_handle(struct drm_prime_file_private >>>> *prime_fpriv, >>>> + uint32_t handle); >>>> /* drm_drv.c */ >>>> struct drm_minor *drm_minor_acquire(unsigned int minor_id); >>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c >>>> index e3f09f18110c..bd5366b16381 100644 >>>> --- a/drivers/gpu/drm/drm_prime.c >>>> +++ b/drivers/gpu/drm/drm_prime.c >>>> @@ -190,29 +190,33 @@ static int drm_prime_lookup_buf_handle(struct >>>> drm_prime_file_private *prime_fpri >>>> return -ENOENT; >>>> } >>>> -void drm_prime_remove_buf_handle_locked(struct >>>> drm_prime_file_private *prime_fpriv, >>>> - struct dma_buf *dma_buf) >>>> +void drm_prime_remove_buf_handle(struct drm_prime_file_private >>>> *prime_fpriv, >>>> + uint32_t handle) >>>> { >>>> struct rb_node *rb; >>>> - rb = prime_fpriv->dmabufs.rb_node; >>>> + mutex_lock(&prime_fpriv->lock); >>>> + >>>> + rb = prime_fpriv->handles.rb_node; >>>> while (rb) { >>>> struct drm_prime_member *member; >>>> - member = rb_entry(rb, struct drm_prime_member, dmabuf_rb); >>>> - if (member->dma_buf == dma_buf) { >>>> + member = rb_entry(rb, struct drm_prime_member, handle_rb); >>>> + if (member->handle == handle) { >>>> rb_erase(&member->handle_rb, &prime_fpriv->handles); >>>> rb_erase(&member->dmabuf_rb, &prime_fpriv->dmabufs); >>>> - dma_buf_put(dma_buf); >>>> + dma_buf_put(member->dma_buf); >>>> kfree(member); >>>> - return; >>>> - } else if (member->dma_buf < dma_buf) { >>>> + break; >>>> + } else if (member->handle < handle) { >>>> rb = rb->rb_right; >>>> } else { >>>> rb = rb->rb_left; >>>> } >>>> } >>>> + >>>> + mutex_unlock(&prime_fpriv->lock); >>>> } >>>> void drm_prime_init_file_private(struct drm_prime_file_private >>>> *prime_fpriv) >>> >>> >> > >
Am 09.08.22 um 03:28 schrieb Chen Jeffy: > Hi Christian, > > On 8/9 星期二 2:03, Christian König wrote: >> Hi Jeffy, >> >> Am 08.08.22 um 05:51 schrieb Chen Jeffy: >>> Hi Christian, >>> >>> Thanks for your reply, and sorry i didn't make it clear. >>> >>> On 8/8 星期一 0:52, Christian König wrote: >>>> Am 03.08.22 um 10:32 schrieb Jeffy Chen: >>>>> Currently we are assuming a one to one mapping between dmabuf and >>>>> handle >>>>> when releasing GEM handles. >>>>> >>>>> But that is not always true, since we would create extra handles >>>>> for the >>>>> GEM obj in cases like gem_open() and getfb{,2}(). >>>>> >>>>> A similar issue was reported at: >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20211105083308.392156-1-jay.xu%40rock-chips.com%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7C52cd6ca16a3a415b92a708da79a67dec%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637956053232922419%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=hIuH18B10sbVAyS0D4iK6R6WYc%2BZ7mlxGcKdUae%2BW6Y%3D&reserved=0 >>>>> >>>>> >>>>> Another problem is that the drm_gem_remove_prime_handles() now only >>>>> remove handle to the exported dmabuf (gem_obj->dma_buf), so the >>>>> imported >>>>> ones would leak: >>>>> WARNING: CPU: 2 PID: 236 at drivers/gpu/drm/drm_prime.c:228 >>>>> drm_prime_destroy_file_private+0x18/0x24 >>>>> >>>>> Let's fix these by using handle to find the exact map to remove. >>>> >>>> Well we are clearly something missing here. As far as I can see the >>>> current code is correct. >>>> >>>> Creating multiple GEM handles for the same DMA-buf is possible, but >>>> illegal. > >>>> In other words when a GEM handle is exported as DMA-buf and >>>> imported again you should intentionally always get the same handle. >>> >>> These issue are not about having handles for importing an exported >>> dma-buf case, but for having multiple handles to a GEM object(which >>> means having multiple handles to a dma-buf). >>> >>> I know the drm-prime is trying to make dma-buf and handle maps one >>> to one, but the drm-gem is allowing to create extra handles for a >>> GEM object, for example: >>> drm_gem_open_ioctl -> drm_gem_handle_create_tail >>> drm_mode_getfb2_ioctl -> drm_gem_handle_create >>> drm_mode_getfb -> fb->funcs->create_handle >> >> Yes, so far that's correct. >> >>> >>> >>> So we are allowing GEM object to have multiple handles, and GEM >>> object could have at most one dma-buf, doesn't that means that >>> dma-buf could map to multiple handles? >> >> No, at least not for the same GEM file private. That's the reason why >> the rb is indexed by the dma_buf object and not the handle. >> >> In other words the rb is so that you have exactly one handle for each >> dma_buf in each file private. > > I don't think so, because if user get multiple handles for the same > GEM obj and use drm_gem_prime_handle_to_fd() for those handles Mhm, that works? This is illegal and should have been prevented somehow. Let me double check the code. Thanks for pointing that out, Christian. > , the current code would try to add multiple maps to rb: > drm_prime_add_buf_handle(buf_1, hdl_1) > drm_prime_add_buf_handle(buf_1, hdl_2) > ... > drm_prime_add_buf_handle(buf_1, hdl_n) > >> >>> >>> Or should we rewrite the GEM framework to limit GEM object with uniq >>> handle? >> >> No, the extra handles are expected because when you call >> drm_mode_getfb*() and drm_gem_open_ioctl() the caller now owns the >> returned GEM handle. >> >>> >>> The other issue is that we are leaking dma-buf <-> handle map for >>> the imported dma-buf, since the drm_gem_remove_prime_handles doesn't >>> take care of obj->import_attach->dmabuf. >> >> No, that's correct as well. obj->dma_buf is set even for imported >> DMA-buf objects. See drm_gem_prime_fd_to_handle(). > > Well, that obj->dma_buf would be set in > drm_gem_prime_fd_to_handle(create new handle), and cleared when > releasing the latest handle(release handle). > > So it doesn't cover other handle creating path. > > For example, a imported dma buf: > drm_gem_prime_fd_to_handle <-- we got a handle and obj->dma_buf and > obj->import_attach->dmabuf > drm_gem_handle_delete <-- we lost that handle and obj->dma_buf cleared > drm_gem_open_ioctl/or getfb* <-- we got a new handle and > obj->import_attach->dmabuf > drm_gem_handle_delete <-- we lost that handle and obj->dma_buf is > null, which means rb leaks. > >> >> Regards, >> Christian. >> >>> >>> But of cause this can be fixed in other way: >>> +++ b/drivers/gpu/drm/drm_gem.c >>> @@ -180,6 +180,9 @@ drm_gem_remove_prime_handles(struct >>> drm_gem_object *obj, struct drm_file *filp) >>> drm_prime_remove_buf_handle_locked(&filp->prime, >>> obj->dma_buf); >>> } >>> + if (obj->import_attach) >>> + drm_prime_remove_buf_handle_locked(&filp->prime, >>> + obj->import_attach->dmabuf); >>> mutex_unlock(&filp->prime.lock); >>> } >>> >>> >>>> So this is pretty much a clear NAK to this patch since it shouldn't >>>> be necessary or something is seriously broken somewhere else. >>>> >>>> Regards, >>>> Christian. >>>> >>>>> >>>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >>>>> --- >>>>> >>>>> Changes in v2: >>>>> Fix a typo of rbtree. >>>>> >>>>> drivers/gpu/drm/drm_gem.c | 17 +---------------- >>>>> drivers/gpu/drm/drm_internal.h | 4 ++-- >>>>> drivers/gpu/drm/drm_prime.c | 20 ++++++++++++-------- >>>>> 3 files changed, 15 insertions(+), 26 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >>>>> index eb0c2d041f13..ed39da383570 100644 >>>>> --- a/drivers/gpu/drm/drm_gem.c >>>>> +++ b/drivers/gpu/drm/drm_gem.c >>>>> @@ -168,21 +168,6 @@ void drm_gem_private_object_init(struct >>>>> drm_device *dev, >>>>> } >>>>> EXPORT_SYMBOL(drm_gem_private_object_init); >>>>> -static void >>>>> -drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct >>>>> drm_file *filp) >>>>> -{ >>>>> - /* >>>>> - * Note: obj->dma_buf can't disappear as long as we still hold a >>>>> - * handle reference in obj->handle_count. >>>>> - */ >>>>> - mutex_lock(&filp->prime.lock); >>>>> - if (obj->dma_buf) { >>>>> - drm_prime_remove_buf_handle_locked(&filp->prime, >>>>> - obj->dma_buf); >>>>> - } >>>>> - mutex_unlock(&filp->prime.lock); >>>>> -} >>>>> - >>>>> /** >>>>> * drm_gem_object_handle_free - release resources bound to >>>>> userspace handles >>>>> * @obj: GEM object to clean up. >>>>> @@ -253,7 +238,7 @@ drm_gem_object_release_handle(int id, void >>>>> *ptr, void *data) >>>>> if (obj->funcs->close) >>>>> obj->funcs->close(obj, file_priv); >>>>> - drm_gem_remove_prime_handles(obj, file_priv); >>>>> + drm_prime_remove_buf_handle(&file_priv->prime, id); >>>>> drm_vma_node_revoke(&obj->vma_node, file_priv); >>>>> drm_gem_object_handle_put_unlocked(obj); >>>>> diff --git a/drivers/gpu/drm/drm_internal.h >>>>> b/drivers/gpu/drm/drm_internal.h >>>>> index 1fbbc19f1ac0..7bb98e6a446d 100644 >>>>> --- a/drivers/gpu/drm/drm_internal.h >>>>> +++ b/drivers/gpu/drm/drm_internal.h >>>>> @@ -74,8 +74,8 @@ int drm_prime_fd_to_handle_ioctl(struct >>>>> drm_device *dev, void *data, >>>>> void drm_prime_init_file_private(struct drm_prime_file_private >>>>> *prime_fpriv); >>>>> void drm_prime_destroy_file_private(struct >>>>> drm_prime_file_private *prime_fpriv); >>>>> -void drm_prime_remove_buf_handle_locked(struct >>>>> drm_prime_file_private *prime_fpriv, >>>>> - struct dma_buf *dma_buf); >>>>> +void drm_prime_remove_buf_handle(struct drm_prime_file_private >>>>> *prime_fpriv, >>>>> + uint32_t handle); >>>>> /* drm_drv.c */ >>>>> struct drm_minor *drm_minor_acquire(unsigned int minor_id); >>>>> diff --git a/drivers/gpu/drm/drm_prime.c >>>>> b/drivers/gpu/drm/drm_prime.c >>>>> index e3f09f18110c..bd5366b16381 100644 >>>>> --- a/drivers/gpu/drm/drm_prime.c >>>>> +++ b/drivers/gpu/drm/drm_prime.c >>>>> @@ -190,29 +190,33 @@ static int >>>>> drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpri >>>>> return -ENOENT; >>>>> } >>>>> -void drm_prime_remove_buf_handle_locked(struct >>>>> drm_prime_file_private *prime_fpriv, >>>>> - struct dma_buf *dma_buf) >>>>> +void drm_prime_remove_buf_handle(struct drm_prime_file_private >>>>> *prime_fpriv, >>>>> + uint32_t handle) >>>>> { >>>>> struct rb_node *rb; >>>>> - rb = prime_fpriv->dmabufs.rb_node; >>>>> + mutex_lock(&prime_fpriv->lock); >>>>> + >>>>> + rb = prime_fpriv->handles.rb_node; >>>>> while (rb) { >>>>> struct drm_prime_member *member; >>>>> - member = rb_entry(rb, struct drm_prime_member, dmabuf_rb); >>>>> - if (member->dma_buf == dma_buf) { >>>>> + member = rb_entry(rb, struct drm_prime_member, handle_rb); >>>>> + if (member->handle == handle) { >>>>> rb_erase(&member->handle_rb, &prime_fpriv->handles); >>>>> rb_erase(&member->dmabuf_rb, &prime_fpriv->dmabufs); >>>>> - dma_buf_put(dma_buf); >>>>> + dma_buf_put(member->dma_buf); >>>>> kfree(member); >>>>> - return; >>>>> - } else if (member->dma_buf < dma_buf) { >>>>> + break; >>>>> + } else if (member->handle < handle) { >>>>> rb = rb->rb_right; >>>>> } else { >>>>> rb = rb->rb_left; >>>>> } >>>>> } >>>>> + >>>>> + mutex_unlock(&prime_fpriv->lock); >>>>> } >>>>> void drm_prime_init_file_private(struct drm_prime_file_private >>>>> *prime_fpriv) >>>> >>>> >>> >> >> >
Hi Christian, On 8/9 星期二 15:55, Christian König wrote: > Am 09.08.22 um 03:28 schrieb Chen Jeffy: >> Hi Christian, >> >> On 8/9 星期二 2:03, Christian König wrote: >>> Hi Jeffy, >>> >>> Am 08.08.22 um 05:51 schrieb Chen Jeffy: >>>> Hi Christian, >>>> >>>> Thanks for your reply, and sorry i didn't make it clear. >>>> >>>> On 8/8 星期一 0:52, Christian König wrote: >>>>> Am 03.08.22 um 10:32 schrieb Jeffy Chen: >>>>>> Currently we are assuming a one to one mapping between dmabuf and >>>>>> handle >>>>>> when releasing GEM handles. >>>>>> >>>>>> But that is not always true, since we would create extra handles >>>>>> for the >>>>>> GEM obj in cases like gem_open() and getfb{,2}(). >>>>>> >>>>>> A similar issue was reported at: >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20211105083308.392156-1-jay.xu%40rock-chips.com%2F&data=05%7C01%7Cchristian.koenig%40amd.com%7C52cd6ca16a3a415b92a708da79a67dec%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637956053232922419%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=hIuH18B10sbVAyS0D4iK6R6WYc%2BZ7mlxGcKdUae%2BW6Y%3D&reserved=0 >>>>>> >>>>>> Another problem is that the drm_gem_remove_prime_handles() now only >>>>>> remove handle to the exported dmabuf (gem_obj->dma_buf), so the >>>>>> imported >>>>>> ones would leak: >>>>>> WARNING: CPU: 2 PID: 236 at drivers/gpu/drm/drm_prime.c:228 >>>>>> drm_prime_destroy_file_private+0x18/0x24 >>>>>> >>>>>> Let's fix these by using handle to find the exact map to remove. >>>>> >>>>> Well we are clearly something missing here. As far as I can see the >>>>> current code is correct. >>>>> >>>>> Creating multiple GEM handles for the same DMA-buf is possible, but >>>>> illegal. > >>>>> In other words when a GEM handle is exported as DMA-buf and >>>>> imported again you should intentionally always get the same handle. >>>> >>>> These issue are not about having handles for importing an exported >>>> dma-buf case, but for having multiple handles to a GEM object(which >>>> means having multiple handles to a dma-buf). >>>> >>>> I know the drm-prime is trying to make dma-buf and handle maps one >>>> to one, but the drm-gem is allowing to create extra handles for a >>>> GEM object, for example: >>>> drm_gem_open_ioctl -> drm_gem_handle_create_tail >>>> drm_mode_getfb2_ioctl -> drm_gem_handle_create >>>> drm_mode_getfb -> fb->funcs->create_handle >>> >>> Yes, so far that's correct. >>> >>>> >>>> >>>> So we are allowing GEM object to have multiple handles, and GEM >>>> object could have at most one dma-buf, doesn't that means that >>>> dma-buf could map to multiple handles? >>> >>> No, at least not for the same GEM file private. That's the reason why >>> the rb is indexed by the dma_buf object and not the handle. >>> >>> In other words the rb is so that you have exactly one handle for each >>> dma_buf in each file private. >> >> I don't think so, because if user get multiple handles for the same >> GEM obj and use drm_gem_prime_handle_to_fd() for those handles > > Mhm, that works? This is illegal and should have been prevented somehow. > > Let me double check the code. > > Thanks for pointing that out, > Christian. > Thanks for checking it, my test case is a preload library which hooks the drmModeSetCrtc(and other APIs) then use drmModeGetFB to extract dmafd from fb_id. > >> , the current code would try to add multiple maps to rb: >> drm_prime_add_buf_handle(buf_1, hdl_1) >> drm_prime_add_buf_handle(buf_1, hdl_2) >> ... >> drm_prime_add_buf_handle(buf_1, hdl_n) >> >>> >>>> >>>> Or should we rewrite the GEM framework to limit GEM object with uniq >>>> handle? >>> >>> No, the extra handles are expected because when you call >>> drm_mode_getfb*() and drm_gem_open_ioctl() the caller now owns the >>> returned GEM handle. >>> >>>> >>>> The other issue is that we are leaking dma-buf <-> handle map for >>>> the imported dma-buf, since the drm_gem_remove_prime_handles doesn't >>>> take care of obj->import_attach->dmabuf. >>> >>> No, that's correct as well. obj->dma_buf is set even for imported >>> DMA-buf objects. See drm_gem_prime_fd_to_handle(). >> >> Well, that obj->dma_buf would be set in >> drm_gem_prime_fd_to_handle(create new handle), and cleared when >> releasing the latest handle(release handle). >> >> So it doesn't cover other handle creating path. >> >> For example, a imported dma buf: >> drm_gem_prime_fd_to_handle <-- we got a handle and obj->dma_buf and >> obj->import_attach->dmabuf >> drm_gem_handle_delete <-- we lost that handle and obj->dma_buf cleared >> drm_gem_open_ioctl/or getfb* <-- we got a new handle and >> obj->import_attach->dmabuf >> drm_gem_handle_delete <-- we lost that handle and obj->dma_buf is >> null, which means rb leaks. Another way to solve this would be set this obj->dma_buf again in drm_gem_prime_handle_to_fd(), which would make sure obj->dma_buf is valid in all current paths lead to drm_prime_add_buf_handle(). >> >>> >>> Regards, >>> Christian. >>> >>>> >>>> But of cause this can be fixed in other way: >>>> +++ b/drivers/gpu/drm/drm_gem.c >>>> @@ -180,6 +180,9 @@ drm_gem_remove_prime_handles(struct >>>> drm_gem_object *obj, struct drm_file *filp) >>>> drm_prime_remove_buf_handle_locked(&filp->prime, >>>> obj->dma_buf); >>>> } >>>> + if (obj->import_attach) >>>> + drm_prime_remove_buf_handle_locked(&filp->prime, >>>> + obj->import_attach->dmabuf); >>>> mutex_unlock(&filp->prime.lock); >>>> } >>>> >>>> >>>>> So this is pretty much a clear NAK to this patch since it shouldn't >>>>> be necessary or something is seriously broken somewhere else. >>>>> >>>>> Regards, >>>>> Christian. >>>>> >>>>>> >>>>>> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >>>>>> --- >>>>>> >>>>>> Changes in v2: >>>>>> Fix a typo of rbtree. >>>>>> >>>>>> drivers/gpu/drm/drm_gem.c | 17 +---------------- >>>>>> drivers/gpu/drm/drm_internal.h | 4 ++-- >>>>>> drivers/gpu/drm/drm_prime.c | 20 ++++++++++++-------- >>>>>> 3 files changed, 15 insertions(+), 26 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c >>>>>> index eb0c2d041f13..ed39da383570 100644 >>>>>> --- a/drivers/gpu/drm/drm_gem.c >>>>>> +++ b/drivers/gpu/drm/drm_gem.c >>>>>> @@ -168,21 +168,6 @@ void drm_gem_private_object_init(struct >>>>>> drm_device *dev, >>>>>> } >>>>>> EXPORT_SYMBOL(drm_gem_private_object_init); >>>>>> -static void >>>>>> -drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct >>>>>> drm_file *filp) >>>>>> -{ >>>>>> - /* >>>>>> - * Note: obj->dma_buf can't disappear as long as we still hold a >>>>>> - * handle reference in obj->handle_count. >>>>>> - */ >>>>>> - mutex_lock(&filp->prime.lock); >>>>>> - if (obj->dma_buf) { >>>>>> - drm_prime_remove_buf_handle_locked(&filp->prime, >>>>>> - obj->dma_buf); >>>>>> - } >>>>>> - mutex_unlock(&filp->prime.lock); >>>>>> -} >>>>>> - >>>>>> /** >>>>>> * drm_gem_object_handle_free - release resources bound to >>>>>> userspace handles >>>>>> * @obj: GEM object to clean up. >>>>>> @@ -253,7 +238,7 @@ drm_gem_object_release_handle(int id, void >>>>>> *ptr, void *data) >>>>>> if (obj->funcs->close) >>>>>> obj->funcs->close(obj, file_priv); >>>>>> - drm_gem_remove_prime_handles(obj, file_priv); >>>>>> + drm_prime_remove_buf_handle(&file_priv->prime, id); >>>>>> drm_vma_node_revoke(&obj->vma_node, file_priv); >>>>>> drm_gem_object_handle_put_unlocked(obj); >>>>>> diff --git a/drivers/gpu/drm/drm_internal.h >>>>>> b/drivers/gpu/drm/drm_internal.h >>>>>> index 1fbbc19f1ac0..7bb98e6a446d 100644 >>>>>> --- a/drivers/gpu/drm/drm_internal.h >>>>>> +++ b/drivers/gpu/drm/drm_internal.h >>>>>> @@ -74,8 +74,8 @@ int drm_prime_fd_to_handle_ioctl(struct >>>>>> drm_device *dev, void *data, >>>>>> void drm_prime_init_file_private(struct drm_prime_file_private >>>>>> *prime_fpriv); >>>>>> void drm_prime_destroy_file_private(struct >>>>>> drm_prime_file_private *prime_fpriv); >>>>>> -void drm_prime_remove_buf_handle_locked(struct >>>>>> drm_prime_file_private *prime_fpriv, >>>>>> - struct dma_buf *dma_buf); >>>>>> +void drm_prime_remove_buf_handle(struct drm_prime_file_private >>>>>> *prime_fpriv, >>>>>> + uint32_t handle); >>>>>> /* drm_drv.c */ >>>>>> struct drm_minor *drm_minor_acquire(unsigned int minor_id); >>>>>> diff --git a/drivers/gpu/drm/drm_prime.c >>>>>> b/drivers/gpu/drm/drm_prime.c >>>>>> index e3f09f18110c..bd5366b16381 100644 >>>>>> --- a/drivers/gpu/drm/drm_prime.c >>>>>> +++ b/drivers/gpu/drm/drm_prime.c >>>>>> @@ -190,29 +190,33 @@ static int >>>>>> drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpri >>>>>> return -ENOENT; >>>>>> } >>>>>> -void drm_prime_remove_buf_handle_locked(struct >>>>>> drm_prime_file_private *prime_fpriv, >>>>>> - struct dma_buf *dma_buf) >>>>>> +void drm_prime_remove_buf_handle(struct drm_prime_file_private >>>>>> *prime_fpriv, >>>>>> + uint32_t handle) >>>>>> { >>>>>> struct rb_node *rb; >>>>>> - rb = prime_fpriv->dmabufs.rb_node; >>>>>> + mutex_lock(&prime_fpriv->lock); >>>>>> + >>>>>> + rb = prime_fpriv->handles.rb_node; >>>>>> while (rb) { >>>>>> struct drm_prime_member *member; >>>>>> - member = rb_entry(rb, struct drm_prime_member, dmabuf_rb); >>>>>> - if (member->dma_buf == dma_buf) { >>>>>> + member = rb_entry(rb, struct drm_prime_member, handle_rb); >>>>>> + if (member->handle == handle) { >>>>>> rb_erase(&member->handle_rb, &prime_fpriv->handles); >>>>>> rb_erase(&member->dmabuf_rb, &prime_fpriv->dmabufs); >>>>>> - dma_buf_put(dma_buf); >>>>>> + dma_buf_put(member->dma_buf); >>>>>> kfree(member); >>>>>> - return; >>>>>> - } else if (member->dma_buf < dma_buf) { >>>>>> + break; >>>>>> + } else if (member->handle < handle) { >>>>>> rb = rb->rb_right; >>>>>> } else { >>>>>> rb = rb->rb_left; >>>>>> } >>>>>> } >>>>>> + >>>>>> + mutex_unlock(&prime_fpriv->lock); >>>>>> } >>>>>> void drm_prime_init_file_private(struct drm_prime_file_private >>>>>> *prime_fpriv) >>>>> >>>>> >>>> >>> >>> >> > >
Hi Jeffy, Am 09.08.22 um 09:55 schrieb Christian König: > [SNIP] >>> >>>> >>>> >>>> So we are allowing GEM object to have multiple handles, and GEM >>>> object could have at most one dma-buf, doesn't that means that >>>> dma-buf could map to multiple handles? >>> >>> No, at least not for the same GEM file private. That's the reason >>> why the rb is indexed by the dma_buf object and not the handle. >>> >>> In other words the rb is so that you have exactly one handle for >>> each dma_buf in each file private. >> >> I don't think so, because if user get multiple handles for the same >> GEM obj and use drm_gem_prime_handle_to_fd() for those handles > > Mhm, that works? This is illegal and should have been prevented somehow. At least I see the problem now. I'm just not sure how to fix it. Your v2 patch indeed prevents leakage of the drm_prime_member for the additional handles, but those shouldn't have been added in the first place. The issue is that with this we make it unpredictable which handle is returned. E.g. if we have handle 2,5,7 it can be that because of re-balancing the tree sometimes 2 and sometimes 5 is returned. That's not really a good idea and breaks a couple of assumptions as far as I know. Ideas? Thanks, Christian.
Hi Christian, On 8/9 星期二 17:08, Christian König wrote: > Hi Jeffy, > > Am 09.08.22 um 09:55 schrieb Christian König: >> [SNIP] >>>> >>>>> >>>>> >>>>> So we are allowing GEM object to have multiple handles, and GEM >>>>> object could have at most one dma-buf, doesn't that means that >>>>> dma-buf could map to multiple handles? >>>> >>>> No, at least not for the same GEM file private. That's the reason >>>> why the rb is indexed by the dma_buf object and not the handle. >>>> >>>> In other words the rb is so that you have exactly one handle for >>>> each dma_buf in each file private. >>> >>> I don't think so, because if user get multiple handles for the same >>> GEM obj and use drm_gem_prime_handle_to_fd() for those handles >> >> Mhm, that works? This is illegal and should have been prevented somehow. > > At least I see the problem now. I'm just not sure how to fix it. > > Your v2 patch indeed prevents leakage of the drm_prime_member for the > additional handles, but those shouldn't have been added in the first place. > > The issue is that with this we make it unpredictable which handle is > returned. E.g. if we have handle 2,5,7 it can be that because of > re-balancing the tree sometimes 2 and sometimes 5 is returned. Maybe cache the latest returned handle in the obj(after drm_gem_prime_fd_to_handle), and clear it when that handle been deleted in drm_gem_handle_delete()? Something like: drm_gem_prime_fd_to_handle handle = drm_prime_lookup_buf_handle(buf) obj = obj_from_handle(handle) if !obj->primary_handle obj->primary_handle = handle return obj->primary_handle Or maybe limit GEM obj with a single lifetime handle? > > That's not really a good idea and breaks a couple of assumptions as far > as I know. > > Ideas? > > Thanks, > Christian. >
Hi Jeffy, Am 09.08.22 um 12:02 schrieb Chen Jeffy: > Hi Christian, > > On 8/9 星期二 17:08, Christian König wrote: >> Hi Jeffy, >> >> Am 09.08.22 um 09:55 schrieb Christian König: >>> [SNIP] >>>>> >>>>>> >>>>>> >>>>>> So we are allowing GEM object to have multiple handles, and GEM >>>>>> object could have at most one dma-buf, doesn't that means that >>>>>> dma-buf could map to multiple handles? >>>>> >>>>> No, at least not for the same GEM file private. That's the reason >>>>> why the rb is indexed by the dma_buf object and not the handle. >>>>> >>>>> In other words the rb is so that you have exactly one handle for >>>>> each dma_buf in each file private. >>>> >>>> I don't think so, because if user get multiple handles for the same >>>> GEM obj and use drm_gem_prime_handle_to_fd() for those handles >>> >>> Mhm, that works? This is illegal and should have been prevented >>> somehow. >> >> At least I see the problem now. I'm just not sure how to fix it. >> >> Your v2 patch indeed prevents leakage of the drm_prime_member for the >> additional handles, but those shouldn't have been added in the first >> place. >> >> The issue is that with this we make it unpredictable which handle is >> returned. E.g. if we have handle 2,5,7 it can be that because of >> re-balancing the tree sometimes 2 and sometimes 5 is returned. > > Maybe cache the latest returned handle in the obj(after > drm_gem_prime_fd_to_handle), and clear it when that handle been > deleted in drm_gem_handle_delete()? That won't work. The handle is per fpriv, but the same object is used by multiple fpriv instances. What we could maybe do is to prevent adding multiple lockup structures when there is already one, but that's not something I can easily judge. Daniel seems to be either very busy or on vacation otherwise he would have chimed in by now. Anyway, your patch seems to at least fix the of hand memory leak, so feel free to add my rb to the v2 and push it to drm-misc-fixes for now. Thanks, Christian. > > > Something like: > drm_gem_prime_fd_to_handle > handle = drm_prime_lookup_buf_handle(buf) > obj = obj_from_handle(handle) > if !obj->primary_handle > obj->primary_handle = handle > return obj->primary_handle > > Or maybe limit GEM obj with a single lifetime handle? > >> >> That's not really a good idea and breaks a couple of assumptions as >> far as I know. >> >> Ideas? >> >> Thanks, >> Christian. >> >
Hi Christian, On 8/9 星期二 18:18, Christian König wrote: > Hi Jeffy, > > Am 09.08.22 um 12:02 schrieb Chen Jeffy: >> Hi Christian, >> >> On 8/9 星期二 17:08, Christian König wrote: >>> Hi Jeffy, >>> >>> Am 09.08.22 um 09:55 schrieb Christian König: >>>> [SNIP] >>>>>> >>>>>>> >>>>>>> >>>>>>> So we are allowing GEM object to have multiple handles, and GEM >>>>>>> object could have at most one dma-buf, doesn't that means that >>>>>>> dma-buf could map to multiple handles? >>>>>> >>>>>> No, at least not for the same GEM file private. That's the reason >>>>>> why the rb is indexed by the dma_buf object and not the handle. >>>>>> >>>>>> In other words the rb is so that you have exactly one handle for >>>>>> each dma_buf in each file private. >>>>> >>>>> I don't think so, because if user get multiple handles for the same >>>>> GEM obj and use drm_gem_prime_handle_to_fd() for those handles >>>> >>>> Mhm, that works? This is illegal and should have been prevented >>>> somehow. >>> >>> At least I see the problem now. I'm just not sure how to fix it. >>> >>> Your v2 patch indeed prevents leakage of the drm_prime_member for the >>> additional handles, but those shouldn't have been added in the first >>> place. >>> >>> The issue is that with this we make it unpredictable which handle is >>> returned. E.g. if we have handle 2,5,7 it can be that because of >>> re-balancing the tree sometimes 2 and sometimes 5 is returned. >> >> Maybe cache the latest returned handle in the obj(after >> drm_gem_prime_fd_to_handle), and clear it when that handle been >> deleted in drm_gem_handle_delete()? > > That won't work. The handle is per fpriv, but the same object is used by > multiple fpriv instances. > > What we could maybe do is to prevent adding multiple lockup structures > when there is already one, but that's not something I can easily judge. So maybe we need to protect that unique lookup structure been deleted before deleting the last handle, and make the handle unique for GEM obj, in case of that unique lookup's handle been deleted earlier that others? How about adding a GEM obj rbtree too, and make drm_prime_member kref-ed? So the drm_prime_add_buf_handle/drm_gem_handle_create_tail/drm_gem_handle_delete would be looking up drm_prime_member by GEM obj, then update dmabuf rb and inc/dec drm_prime_member's kref, drm_gem_prime_fd_to_handle/drm_gem_prime_handle_to_fd remain unchanged. > > Daniel seems to be either very busy or on vacation otherwise he would > have chimed in by now. > > Anyway, your patch seems to at least fix the of hand memory leak, so > feel free to add my rb to the v2 and push it to drm-misc-fixes for now. > > Thanks, > Christian. > >> >> >> Something like: >> drm_gem_prime_fd_to_handle >> handle = drm_prime_lookup_buf_handle(buf) >> obj = obj_from_handle(handle) >> if !obj->primary_handle >> obj->primary_handle = handle >> return obj->primary_handle >> >> Or maybe limit GEM obj with a single lifetime handle? >> >>> >>> That's not really a good idea and breaks a couple of assumptions as >>> far as I know. >>> >>> Ideas? >>> >>> Thanks, >>> Christian. >>> >> > >
Hi Jeffy, Am 10.08.22 um 06:16 schrieb Chen Jeffy: > Hi Christian, > > On 8/9 星期二 18:18, Christian König wrote: >> Hi Jeffy, >> [SNIP] >>> Maybe cache the latest returned handle in the obj(after >>> drm_gem_prime_fd_to_handle), and clear it when that handle been >>> deleted in drm_gem_handle_delete()? >> >> That won't work. The handle is per fpriv, but the same object is used >> by multiple fpriv instances. > >> What we could maybe do is to prevent adding multiple lockup >> structures when there is already one, but that's not something I can >> easily judge. > > So maybe we need to protect that unique lookup structure been deleted > before deleting the last handle, and make the handle unique for GEM > obj, in case of that unique lookup's handle been deleted earlier that > others? > > How about adding a GEM obj rbtree too, and make drm_prime_member kref-ed? > > So the > drm_prime_add_buf_handle/drm_gem_handle_create_tail/drm_gem_handle_delete > would be looking up drm_prime_member by GEM obj, then update dmabuf rb > and inc/dec drm_prime_member's kref, > drm_gem_prime_fd_to_handle/drm_gem_prime_handle_to_fd remain unchanged. I think we should probably come up with an idea what the UAPI should look like before we try to implement this in the kernel, but in general I think we should make the solution simpler and not even more complex. Recording multiple handles for the same DMA-buf/fpriv combination doesn't seem to make sense, so the duplicated tracking of handle->dma_buf mapping just seems to be overkill. So my proposal would be this: 1. Only the first used GEM handle is tracker for each DMA-buf/fpriv combination. 2. Imports either return this first used handle or allocate a new one if there isn't any. 3. If the first used handle is closed we allocate a new one on re-import even when there duplicate handles. The alternative as we have it now is to just return a more or less random handle if there are duplicates which doesn't sound like something we would want. Daniel, can we agree on that? Regards, Christian.
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index eb0c2d041f13..ed39da383570 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -168,21 +168,6 @@ void drm_gem_private_object_init(struct drm_device *dev, } EXPORT_SYMBOL(drm_gem_private_object_init); -static void -drm_gem_remove_prime_handles(struct drm_gem_object *obj, struct drm_file *filp) -{ - /* - * Note: obj->dma_buf can't disappear as long as we still hold a - * handle reference in obj->handle_count. - */ - mutex_lock(&filp->prime.lock); - if (obj->dma_buf) { - drm_prime_remove_buf_handle_locked(&filp->prime, - obj->dma_buf); - } - mutex_unlock(&filp->prime.lock); -} - /** * drm_gem_object_handle_free - release resources bound to userspace handles * @obj: GEM object to clean up. @@ -253,7 +238,7 @@ drm_gem_object_release_handle(int id, void *ptr, void *data) if (obj->funcs->close) obj->funcs->close(obj, file_priv); - drm_gem_remove_prime_handles(obj, file_priv); + drm_prime_remove_buf_handle(&file_priv->prime, id); drm_vma_node_revoke(&obj->vma_node, file_priv); drm_gem_object_handle_put_unlocked(obj); diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 1fbbc19f1ac0..7bb98e6a446d 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -74,8 +74,8 @@ int drm_prime_fd_to_handle_ioctl(struct drm_device *dev, void *data, void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv); void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv); -void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv, - struct dma_buf *dma_buf); +void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, + uint32_t handle); /* drm_drv.c */ struct drm_minor *drm_minor_acquire(unsigned int minor_id); diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index e3f09f18110c..bd5366b16381 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -190,29 +190,33 @@ static int drm_prime_lookup_buf_handle(struct drm_prime_file_private *prime_fpri return -ENOENT; } -void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv, - struct dma_buf *dma_buf) +void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, + uint32_t handle) { struct rb_node *rb; - rb = prime_fpriv->dmabufs.rb_node; + mutex_lock(&prime_fpriv->lock); + + rb = prime_fpriv->handles.rb_node; while (rb) { struct drm_prime_member *member; - member = rb_entry(rb, struct drm_prime_member, dmabuf_rb); - if (member->dma_buf == dma_buf) { + member = rb_entry(rb, struct drm_prime_member, handle_rb); + if (member->handle == handle) { rb_erase(&member->handle_rb, &prime_fpriv->handles); rb_erase(&member->dmabuf_rb, &prime_fpriv->dmabufs); - dma_buf_put(dma_buf); + dma_buf_put(member->dma_buf); kfree(member); - return; - } else if (member->dma_buf < dma_buf) { + break; + } else if (member->handle < handle) { rb = rb->rb_right; } else { rb = rb->rb_left; } } + + mutex_unlock(&prime_fpriv->lock); } void drm_prime_init_file_private(struct drm_prime_file_private *prime_fpriv)
Currently we are assuming a one to one mapping between dmabuf and handle when releasing GEM handles. But that is not always true, since we would create extra handles for the GEM obj in cases like gem_open() and getfb{,2}(). A similar issue was reported at: https://lore.kernel.org/all/20211105083308.392156-1-jay.xu@rock-chips.com/ Another problem is that the drm_gem_remove_prime_handles() now only remove handle to the exported dmabuf (gem_obj->dma_buf), so the imported ones would leak: WARNING: CPU: 2 PID: 236 at drivers/gpu/drm/drm_prime.c:228 drm_prime_destroy_file_private+0x18/0x24 Let's fix these by using handle to find the exact map to remove. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> --- Changes in v2: Fix a typo of rbtree. drivers/gpu/drm/drm_gem.c | 17 +---------------- drivers/gpu/drm/drm_internal.h | 4 ++-- drivers/gpu/drm/drm_prime.c | 20 ++++++++++++-------- 3 files changed, 15 insertions(+), 26 deletions(-)