On Wed, May 28, 2025 at 11:13:06AM +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.
>
> On top of that this code also drops the drm_file.table_lock lock while
> iterating, which can mess up the iterator state. And that's actually
> bad.
So I re-read idr_get_next and all that, and I think it should be all fine
- it handles both NULL entries and I think does recover from simply the
most recent id. Might miss some that have been concurrently added, but
that should be fine.
Sorry for the noise.
-Sima
>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> Signed-off-by: Simona Vetter <simona.vet...@ffwll.ch>
> Cc: Lucas De Marchi <lucas.demar...@intel.com>
> Cc: "Thomas Hellström" <thomas.hellst...@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> Cc: intel...@lists.freedesktop.org
> ---
> drivers/gpu/drm/xe/xe_drm_client.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_drm_client.c
> b/drivers/gpu/drm/xe/xe_drm_client.c
> index 31f688e953d7..2542f265a221 100644
> --- a/drivers/gpu/drm/xe/xe_drm_client.c
> +++ b/drivers/gpu/drm/xe/xe_drm_client.c
> @@ -205,6 +205,7 @@ static void show_meminfo(struct drm_printer *p, struct
> drm_file *file)
>
> /* Public objects. */
> spin_lock(&file->table_lock);
> + /* FIXME: Use idr_for_each to handle transient NULL pointers */
> idr_for_each_entry(&file->object_idr, obj, id) {
> struct xe_bo *bo = gem_to_xe_bo(obj);
>
> @@ -213,6 +214,8 @@ static void show_meminfo(struct drm_printer *p, struct
> drm_file *file)
> xe_bo_unlock(bo);
> } else {
> xe_bo_get(bo);
> + /* FIXME: dropping the lock can mess the idr iterator
> + * state up */
> spin_unlock(&file->table_lock);
>
> xe_bo_lock(bo, false);
> --
> 2.49.0
>
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
