Re: [PATCH] drm/panfrost: Fix a deadlock between the shrinker and madvise path

[Steven Price <steven.price@xxxxxxx>]

On 01/11/2020 17:40, Boris Brezillon wrote:
> panfrost_ioctl_madvise() and panfrost_gem_purge() acquire the mappings
> and shmem locks in different orders, thus leading to a potential
> the mappings lock first.
>
> Fixes: bdefca2d8dc0 ("drm/panfrost: Add the panfrost_gem_mapping concept")
> Cc: <stable@xxxxxxxxxxxxxxx>
> Cc: Christian Hewitt <christianshewitt@xxxxxxxxx>
> Reported-by: Christian Hewitt <christianshewitt@xxxxxxxxx>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>

Reviewed-by: Steven Price <steven.price@xxxxxxx>

> ---
>   drivers/gpu/drm/panfrost/panfrost_gem.c          |  4 +---
>   drivers/gpu/drm/panfrost/panfrost_gem.h          |  2 +-
>   drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c | 14 +++++++++++---
>   3 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index cdf1a8754eba..0c0243eaee81 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -105,14 +105,12 @@ void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping)
>   	kref_put(&mapping->refcount, panfrost_gem_mapping_release);
>   }
>   
> -void panfrost_gem_teardown_mappings(struct panfrost_gem_object *bo)
> +void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo)
>   {
>   	struct panfrost_gem_mapping *mapping;
>   
> -	mutex_lock(&bo->mappings.lock);
>   	list_for_each_entry(mapping, &bo->mappings.list, node)
>   		panfrost_gem_teardown_mapping(mapping);
> -	mutex_unlock(&bo->mappings.lock);
>   }
>   
>   int panfrost_gem_open(struct drm_gem_object *obj, struct drm_file *file_priv)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index b3517ff9630c..8088d5fd8480 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -82,7 +82,7 @@ struct panfrost_gem_mapping *
>   panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
>   			 struct panfrost_file_priv *priv);
>   void panfrost_gem_mapping_put(struct panfrost_gem_mapping *mapping);
> -void panfrost_gem_teardown_mappings(struct panfrost_gem_object *bo);
> +void panfrost_gem_teardown_mappings_locked(struct panfrost_gem_object *bo);
>   
>   void panfrost_gem_shrinker_init(struct drm_device *dev);
>   void panfrost_gem_shrinker_cleanup(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> index 288e46c40673..1b9f68d8e9aa 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
> @@ -40,18 +40,26 @@ static bool panfrost_gem_purge(struct drm_gem_object *obj)
>   {
>   	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>   	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +	bool ret = false;
>   
>   	if (atomic_read(&bo->gpu_usecount))
>   		return false;
>   
> -	if (!mutex_trylock(&shmem->pages_lock))
> +	if (!mutex_trylock(&bo->mappings.lock))
>   		return false;
>   
> -	panfrost_gem_teardown_mappings(bo);
> +	if (!mutex_trylock(&shmem->pages_lock))
> +		goto unlock_mappings;
> +
> +	panfrost_gem_teardown_mappings_locked(bo);
>   	drm_gem_shmem_purge_locked(obj);
> +	ret = true;
>   
>   	mutex_unlock(&shmem->pages_lock);
> -	return true;
> +
> +unlock_mappings:
> +	mutex_unlock(&bo->mappings.lock);
> +	return ret;
>   }
>   
>   static unsigned long