Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt()

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4358 bytes --]

Hi

Am 05.02.23 um 13:51 schrieb Asahi Lina:
> Other functions touching shmem->sgt take the pages lock, so do that here

Really? I was just locking at the Lima driver and it apparently access 
sgt without locking in [1]. Not that I disagree with the patch in general.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/lima/lima_gem.c#L21

> too. drm_gem_shmem_get_pages() & co take the same lock, so move to the
> _locked() variants to avoid recursive locking.
> 
> Discovered while auditing locking to write the Rust abstractions.
> 
> Fixes: 2194a63a818d ("drm: Add library for shmem backed GEM objects")
> Fixes: 4fa3d66f132b ("drm/shmem: Do dma_unmap_sg before purging pages")
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> ---
>   drivers/gpu/drm/drm_gem_shmem_helper.c | 54 ++++++++++++++++----------
>   1 file changed, 34 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index b602cd72a120..2c559b310cad 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -681,23 +681,7 @@ struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem)
>   }
>   EXPORT_SYMBOL_GPL(drm_gem_shmem_get_sg_table);
>   
> -/**
> - * drm_gem_shmem_get_pages_sgt - Pin pages, dma map them, and return a
> - *				 scatter/gather table for a shmem GEM object.
> - * @shmem: shmem GEM object
> - *
> - * This function returns a scatter/gather table suitable for driver usage. If
> - * the sg table doesn't exist, the pages are pinned, dma-mapped, and a sg
> - * table created.
> - *
> - * This is the main function for drivers to get at backing storage, and it hides
> - * and difference between dma-buf imported and natively allocated objects.
> - * drm_gem_shmem_get_sg_table() should not be directly called by drivers.
> - *
> - * Returns:
> - * A pointer to the scatter/gather table of pinned pages or errno on failure.
> - */
> -struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
> +static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_object *shmem)
>   {
>   	struct drm_gem_object *obj = &shmem->base;
>   	int ret;
> @@ -708,7 +692,7 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
>   
>   	WARN_ON(obj->import_attach);
>   
> -	ret = drm_gem_shmem_get_pages(shmem);
> +	ret = drm_gem_shmem_get_pages_locked(shmem);
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> @@ -730,10 +714,40 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
>   	sg_free_table(sgt);
>   	kfree(sgt);
>   err_put_pages:
> -	drm_gem_shmem_put_pages(shmem);
> +	drm_gem_shmem_put_pages_locked(shmem);
>   	return ERR_PTR(ret);
>   }
> -EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt);
> +
> +/**
> + * drm_gem_shmem_get_pages_sgt - Pin pages, dma map them, and return a
> + *				 scatter/gather table for a shmem GEM object.
> + * @shmem: shmem GEM object
> + *
> + * This function returns a scatter/gather table suitable for driver usage. If
> + * the sg table doesn't exist, the pages are pinned, dma-mapped, and a sg
> + * table created.
> + *
> + * This is the main function for drivers to get at backing storage, and it hides
> + * and difference between dma-buf imported and natively allocated objects.
> + * drm_gem_shmem_get_sg_table() should not be directly called by drivers.
> + *
> + * Returns:
> + * A pointer to the scatter/gather table of pinned pages or errno on failure.
> + */
> +struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem)
> +{
> +	int ret;
> +	struct sg_table *sgt;
> +
> +	ret = mutex_lock_interruptible(&shmem->pages_lock);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	sgt = drm_gem_shmem_get_pages_sgt_locked(shmem);
> +	mutex_unlock(&shmem->pages_lock);
> +
> +	return sgt;
> +}
> +EXPORT_SYMBOL(drm_gem_shmem_get_pages_sgt);
>   
>   /**
>    * drm_gem_shmem_prime_import_sg_table - Produce a shmem GEM object from

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]