Re: [PATCH] drm/panthor: Fix race when converting group handle to group object

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 23 Sep 2024 11:34:06 +0100
Steven Price <steven.price@arm.com> wrote:

> XArray provides it's own internal lock which protects the internal array
> when entries are being simultaneously added and removed. However there
> is still a race between retrieving the pointer from the XArray and
> incrementing the reference count.
> 
> To avoid this race simply hold the internal XArray lock when
> incrementing the reference count, this ensures there cannot be a racing
> call to xa_erase().
> 
> Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
> Signed-off-by: Steven Price <steven.price@arm.com>

Uh, nice catch!

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 42afdf0ddb7e..0dbeebcf23b4 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -3242,6 +3242,18 @@ int panthor_group_destroy(struct panthor_file *pfile, u32 group_handle)
>  	return 0;
>  }
>  
> +static struct panthor_group *group_from_handle(struct panthor_group_pool *pool,
> +					       u32 group_handle)
> +{
> +	struct panthor_group *group;
> +
> +	xa_lock(&pool->xa);
> +	group = group_get(xa_load(&pool->xa, group_handle));
> +	xa_unlock(&pool->xa);
> +
> +	return group;
> +}
> +
>  int panthor_group_get_state(struct panthor_file *pfile,
>  			    struct drm_panthor_group_get_state *get_state)
>  {
> @@ -3253,7 +3265,7 @@ int panthor_group_get_state(struct panthor_file *pfile,
>  	if (get_state->pad)
>  		return -EINVAL;
>  
> -	group = group_get(xa_load(&gpool->xa, get_state->group_handle));
> +	group = group_from_handle(gpool, get_state->group_handle);
>  	if (!group)
>  		return -EINVAL;
>  
> @@ -3384,7 +3396,7 @@ panthor_job_create(struct panthor_file *pfile,
>  	job->call_info.latest_flush = qsubmit->latest_flush;
>  	INIT_LIST_HEAD(&job->node);
>  
> -	job->group = group_get(xa_load(&gpool->xa, group_handle));
> +	job->group = group_from_handle(gpool, group_handle);
>  	if (!job->group) {
>  		ret = -EINVAL;
>  		goto err_put_job;