diff mbox series

drm/shme-helpers: Fix dma_buf_mmap forwarding bug

Message ID 20201027214922.3566743-1-daniel.vetter@ffwll.ch
State Accepted
Commit f49a51bfdc8ea717c97ccd4cc98b7e6daaa5553a
Headers show
Series drm/shme-helpers: Fix dma_buf_mmap forwarding bug | expand

Commit Message

Daniel Vetter Oct. 27, 2020, 9:49 p.m. UTC
When we forward an mmap to the dma_buf exporter, they get to own
everything. Unfortunately drm_gem_mmap_obj() overwrote
vma->vm_private_data after the driver callback, wreaking the
exporter complete. This was noticed because vb2_common_vm_close blew
up on mali gpu with panfrost after commit 26d3ac3cb04d
("drm/shmem-helpers: Redirect mmap for imported dma-buf").

Unfortunately drm_gem_mmap_obj also acquires a surplus reference that
we need to drop in shmem helpers, which is a bit of a mislayer
situation. Maybe the entire dma_buf_mmap forwarding should be pulled
into core gem code.

Note that the only two other drivers which forward mmap in their own
code (etnaviv and exynos) get this somewhat right by overwriting the
gem mmap code. But they seem to still have the leak. This might be a
good excuse to move these drivers over to shmem helpers completely.

Note to stable team: There's a trivial context conflict with
d693def4fd1c ("drm: Remove obsolete GEM and PRIME callbacks from
struct drm_driver"). I'm assuming it's clear where to put the first
hunk, otherwise I can send a 5.9 version of this.

Cc: Christian König <christian.koenig@amd.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Russell King <linux+etnaviv@armlinux.org.uk>
Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
Cc: Inki Dae <inki.dae@samsung.com>
Cc: Joonyoung Shim <jy0922.shim@samsung.com>
Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf")
Cc: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rob Herring <robh@kernel.org>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: <stable@vger.kernel.org> # v5.9+
Reported-and-tested-by: piotr.oniszczuk@gmail.com
Cc: piotr.oniszczuk@gmail.com
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_gem.c              | 4 ++--
 drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

Comments

Boris Brezillon Oct. 28, 2020, 8:44 a.m. UTC | #1
On Tue, 27 Oct 2020 22:49:22 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> When we forward an mmap to the dma_buf exporter, they get to own
> everything. Unfortunately drm_gem_mmap_obj() overwrote
> vma->vm_private_data after the driver callback, wreaking the
> exporter complete. This was noticed because vb2_common_vm_close blew
> up on mali gpu with panfrost after commit 26d3ac3cb04d
> ("drm/shmem-helpers: Redirect mmap for imported dma-buf").
> 
> Unfortunately drm_gem_mmap_obj also acquires a surplus reference that
> we need to drop in shmem helpers, which is a bit of a mislayer
> situation. Maybe the entire dma_buf_mmap forwarding should be pulled
> into core gem code.
> 
> Note that the only two other drivers which forward mmap in their own
> code (etnaviv and exynos) get this somewhat right by overwriting the
> gem mmap code. But they seem to still have the leak. This might be a
> good excuse to move these drivers over to shmem helpers completely.
> 
> Note to stable team: There's a trivial context conflict with
> d693def4fd1c ("drm: Remove obsolete GEM and PRIME callbacks from
> struct drm_driver"). I'm assuming it's clear where to put the first
> hunk, otherwise I can send a 5.9 version of this.
> 
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf")
> Cc: Boris Brezillon <boris.brezillon@collabora.com>

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

> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: <stable@vger.kernel.org> # v5.9+
> Reported-and-tested-by: piotr.oniszczuk@gmail.com
> Cc: piotr.oniszczuk@gmail.com
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_gem.c              | 4 ++--
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 1da67d34e55d..d586068f5509 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1076,6 +1076,8 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  	 */
>  	drm_gem_object_get(obj);
>  
> +	vma->vm_private_data = obj;
> +
>  	if (obj->funcs->mmap) {
>  		ret = obj->funcs->mmap(obj, vma);
>  		if (ret) {
> @@ -1096,8 +1098,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>  		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>  	}
>  
> -	vma->vm_private_data = obj;
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_mmap_obj);
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index fb11df7aced5..8233bda4692f 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -598,8 +598,13 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  	/* Remove the fake offset */
>  	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>  
> -	if (obj->import_attach)
> +	if (obj->import_attach) {
> +		/* Drop the reference drm_gem_mmap_obj() acquired.*/
> +		drm_gem_object_put(obj);
> +		vma->vm_private_data = NULL;
> +
>  		return dma_buf_mmap(obj->dma_buf, vma, 0);
> +	}
>  
>  	shmem = to_drm_gem_shmem_obj(obj);
>
Christian König Oct. 28, 2020, 9:57 a.m. UTC | #2
Am 27.10.20 um 22:49 schrieb Daniel Vetter:
> When we forward an mmap to the dma_buf exporter, they get to own
> everything. Unfortunately drm_gem_mmap_obj() overwrote
> vma->vm_private_data after the driver callback, wreaking the
> exporter complete. This was noticed because vb2_common_vm_close blew
> up on mali gpu with panfrost after commit 26d3ac3cb04d
> ("drm/shmem-helpers: Redirect mmap for imported dma-buf").
>
> Unfortunately drm_gem_mmap_obj also acquires a surplus reference that
> we need to drop in shmem helpers, which is a bit of a mislayer
> situation. Maybe the entire dma_buf_mmap forwarding should be pulled
> into core gem code.
>
> Note that the only two other drivers which forward mmap in their own
> code (etnaviv and exynos) get this somewhat right by overwriting the
> gem mmap code. But they seem to still have the leak. This might be a
> good excuse to move these drivers over to shmem helpers completely.
>
> Note to stable team: There's a trivial context conflict with
> d693def4fd1c ("drm: Remove obsolete GEM and PRIME callbacks from
> struct drm_driver"). I'm assuming it's clear where to put the first
> hunk, otherwise I can send a 5.9 version of this.
>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf")
> Cc: Boris Brezillon <boris.brezillon@collabora.com>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: <stable@vger.kernel.org> # v5.9+
> Reported-and-tested-by: piotr.oniszczuk@gmail.com
> Cc: piotr.oniszczuk@gmail.com
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Acked-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/drm_gem.c              | 4 ++--
>   drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++-
>   2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 1da67d34e55d..d586068f5509 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1076,6 +1076,8 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>   	 */
>   	drm_gem_object_get(obj);
>   
> +	vma->vm_private_data = obj;
> +
>   	if (obj->funcs->mmap) {
>   		ret = obj->funcs->mmap(obj, vma);
>   		if (ret) {
> @@ -1096,8 +1098,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>   		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>   	}
>   
> -	vma->vm_private_data = obj;
> -
>   	return 0;
>   }
>   EXPORT_SYMBOL(drm_gem_mmap_obj);
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index fb11df7aced5..8233bda4692f 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -598,8 +598,13 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>   	/* Remove the fake offset */
>   	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>   
> -	if (obj->import_attach)
> +	if (obj->import_attach) {
> +		/* Drop the reference drm_gem_mmap_obj() acquired.*/
> +		drm_gem_object_put(obj);
> +		vma->vm_private_data = NULL;
> +
>   		return dma_buf_mmap(obj->dma_buf, vma, 0);
> +	}
>   
>   	shmem = to_drm_gem_shmem_obj(obj);
>
Daniel Vetter Oct. 28, 2020, 10:17 a.m. UTC | #3
On Wed, Oct 28, 2020 at 09:44:15AM +0100, Boris Brezillon wrote:
> On Tue, 27 Oct 2020 22:49:22 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > When we forward an mmap to the dma_buf exporter, they get to own
> > everything. Unfortunately drm_gem_mmap_obj() overwrote
> > vma->vm_private_data after the driver callback, wreaking the
> > exporter complete. This was noticed because vb2_common_vm_close blew
> > up on mali gpu with panfrost after commit 26d3ac3cb04d
> > ("drm/shmem-helpers: Redirect mmap for imported dma-buf").
> > 
> > Unfortunately drm_gem_mmap_obj also acquires a surplus reference that
> > we need to drop in shmem helpers, which is a bit of a mislayer
> > situation. Maybe the entire dma_buf_mmap forwarding should be pulled
> > into core gem code.
> > 
> > Note that the only two other drivers which forward mmap in their own
> > code (etnaviv and exynos) get this somewhat right by overwriting the
> > gem mmap code. But they seem to still have the leak. This might be a
> > good excuse to move these drivers over to shmem helpers completely.
> > 
> > Note to stable team: There's a trivial context conflict with
> > d693def4fd1c ("drm: Remove obsolete GEM and PRIME callbacks from
> > struct drm_driver"). I'm assuming it's clear where to put the first
> > hunk, otherwise I can send a 5.9 version of this.
> > 
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Sumit Semwal <sumit.semwal@linaro.org>
> > Cc: Lucas Stach <l.stach@pengutronix.de>
> > Cc: Russell King <linux+etnaviv@armlinux.org.uk>
> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com>
> > Cc: Inki Dae <inki.dae@samsung.com>
> > Cc: Joonyoung Shim <jy0922.shim@samsung.com>
> > Cc: Seung-Woo Kim <sw0312.kim@samsung.com>
> > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > Fixes: 26d3ac3cb04d ("drm/shmem-helpers: Redirect mmap for imported dma-buf")
> > Cc: Boris Brezillon <boris.brezillon@collabora.com>
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Patch pushed to drm-misc-fixes, thanks for taking a look.
-Daniel

> 
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-media@vger.kernel.org
> > Cc: linaro-mm-sig@lists.linaro.org
> > Cc: <stable@vger.kernel.org> # v5.9+
> > Reported-and-tested-by: piotr.oniszczuk@gmail.com
> > Cc: piotr.oniszczuk@gmail.com
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_gem.c              | 4 ++--
> >  drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++++-
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 1da67d34e55d..d586068f5509 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1076,6 +1076,8 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
> >  	 */
> >  	drm_gem_object_get(obj);
> >  
> > +	vma->vm_private_data = obj;
> > +
> >  	if (obj->funcs->mmap) {
> >  		ret = obj->funcs->mmap(obj, vma);
> >  		if (ret) {
> > @@ -1096,8 +1098,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
> >  		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> >  	}
> >  
> > -	vma->vm_private_data = obj;
> > -
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(drm_gem_mmap_obj);
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index fb11df7aced5..8233bda4692f 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -598,8 +598,13 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> >  	/* Remove the fake offset */
> >  	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
> >  
> > -	if (obj->import_attach)
> > +	if (obj->import_attach) {
> > +		/* Drop the reference drm_gem_mmap_obj() acquired.*/
> > +		drm_gem_object_put(obj);
> > +		vma->vm_private_data = NULL;
> > +
> >  		return dma_buf_mmap(obj->dma_buf, vma, 0);
> > +	}
> >  
> >  	shmem = to_drm_gem_shmem_obj(obj);
> >  
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 1da67d34e55d..d586068f5509 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1076,6 +1076,8 @@  int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 	 */
 	drm_gem_object_get(obj);
 
+	vma->vm_private_data = obj;
+
 	if (obj->funcs->mmap) {
 		ret = obj->funcs->mmap(obj, vma);
 		if (ret) {
@@ -1096,8 +1098,6 @@  int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
 		vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
 	}
 
-	vma->vm_private_data = obj;
-
 	return 0;
 }
 EXPORT_SYMBOL(drm_gem_mmap_obj);
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index fb11df7aced5..8233bda4692f 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -598,8 +598,13 @@  int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 	/* Remove the fake offset */
 	vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
 
-	if (obj->import_attach)
+	if (obj->import_attach) {
+		/* Drop the reference drm_gem_mmap_obj() acquired.*/
+		drm_gem_object_put(obj);
+		vma->vm_private_data = NULL;
+
 		return dma_buf_mmap(obj->dma_buf, vma, 0);
+	}
 
 	shmem = to_drm_gem_shmem_obj(obj);