Re: [PATCH 8/8] drm/xe: Add comments about drm_file.object_idr issues

[Simona Vetter <simona.vetter@ffwll.ch>]

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.vetter@intel.com>
> Signed-off-by: Simona Vetter <simona.vetter@ffwll.ch>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: intel-xe@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