On Wed, May 28, 2025 at 11:13:04AM +0200, Simona Vetter wrote:
> idr_for_each_entry() is fine, but will prematurely terminate on
> transient NULL entries. It should be switched over to idr_for_each,
> which allows you to handle this explicitly.
>
> Note that transient NULL pointers in drm_file.object_idr have been a
> thing since f6cd7daecff5 ("drm: Release driver references to handle
> before making it available again"), this is a really old issue.
>
> Since it's just a premature loop terminate the impact should be fairly
> benign, at least for any debugfs or fdinfo code.
Misread idr_get_next and I now think it should be fine as-is. Please
disregard this one.
-Sima
>
> Aside: amdgpu_gem_force_release() looks questionable and should
> probably be revisited in the light of the revised hotunplug design
> we're aiming for. But that's an entirely separate can of worms.
>
> Cc: Alex Deucher <[email protected]>
> Cc: "Christian König" <[email protected]>
> Cc: Arvind Yadav <[email protected]>
> Cc: Shashank Sharma <[email protected]>
> Cc: Simona Vetter <[email protected]>
> Cc: Yunxiang Li <[email protected]>
> Cc: Frank Min <[email protected]>
> Cc: Kent Russell <[email protected]>
> Signed-off-by: Simona Vetter <[email protected]>
> Signed-off-by: Simona Vetter <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 2c68118fe9fd..90723b13fa7d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -249,6 +249,7 @@ void amdgpu_gem_force_release(struct amdgpu_device *adev)
>
> WARN_ONCE(1, "Still active user space clients!\n");
> spin_lock(&file->table_lock);
> + /* FIXME: Use idr_for_each to handle transient NULL pointers */
> idr_for_each_entry(&file->object_idr, gobj, handle) {
> WARN_ONCE(1, "And also active allocations!\n");
> drm_gem_object_put(gobj);
> @@ -1167,6 +1168,7 @@ static int amdgpu_debugfs_gem_info_show(struct seq_file
> *m, void *unused)
> rcu_read_unlock();
>
> spin_lock(&file->table_lock);
> + /* FIXME: Use idr_for_each to handle transient NULL pointers */
> idr_for_each_entry(&file->object_idr, gobj, id) {
> struct amdgpu_bo *bo = gem_to_amdgpu_bo(gobj);
>
> --
> 2.49.0
>
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
