linux-kernel - Re: [PATCH] drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <77be28c1-52ff-87c8-b7f7-f99273d48267@suse.de>
Date:   Tue, 7 Feb 2023 12:29:52 +0100
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Asahi Lina <lina@...hilina.net>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Noralf Trønnes <noralf@...nnes.org>,
        asahi@...ts.linux.dev, Alyssa Rosenzweig <alyssa@...enzweig.io>
Subject: Re: [PATCH] drm/shmem-helper: Fix locking for
 drm_gem_shmem_get_pages_sgt()

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@...hilina.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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)