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(-)
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);
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);
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);
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);
linux-stable-mirror@lists.linaro.org
-
Boris Brezillon -
Christian König -
Daniel Vetter -
Daniel Vetter