When using mmap() on a prime imported buffer allocated by a different driver (such as imx-drm) the later munmap() does not correctly decrement the refcount of the original enaviv_gem_object, leading to a leak.
Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group Cc: stable@vger.kernel.org --- drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c index f24dd21..28a01b8 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c @@ -93,7 +93,25 @@ static void *etnaviv_gem_prime_vmap_impl(struct etnaviv_gem_object *etnaviv_obj) static int etnaviv_gem_prime_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, struct vm_area_struct *vma) { - return dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0); + int ret; + + ret = dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0); + + /* drm_gem_mmap_obj() has already been called before this function + * and has incremented our refcount, expecting it to be decremented + * on unmap() via drm_gem_vm_close(). + * However dma_buf_mmap() invokes drm_gem_cma_prime_mmap() + * that ends up updating the vma->vma_private_data to point to the + * dma_buf's gem object. + * Hence our gem object here will not have its refcount decremented + * when userspace does unmap(). + * So decrement the refcount here to avoid a memory leak if the dma + * buf mapping was successful. + */ + if (!ret) + drm_gem_object_put_unlocked(&etnaviv_obj->base); + + return ret; }
static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
Hi Martin,
Am Mittwoch, den 20.05.2020, 12:10 +0200 schrieb Martin Fuzzey:
When using mmap() on a prime imported buffer allocated by a different driver (such as imx-drm) the later munmap() does not correctly decrement the refcount of the original enaviv_gem_object, leading to a leak.
Signed-off-by: Martin Fuzzey martin.fuzzey@flowbird.group Cc: stable@vger.kernel.org
What's the use-case where you did hit this issue? mmap'ing of imported buffers through the etnaviv DRM device is currently not well defined and I was pondering the idea of forbidding it completely by not returning a mmap offset for those objects.
Regards, Lucas
drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c index f24dd21..28a01b8 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c @@ -93,7 +93,25 @@ static void *etnaviv_gem_prime_vmap_impl(struct etnaviv_gem_object *etnaviv_obj) static int etnaviv_gem_prime_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, struct vm_area_struct *vma) {
- return dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0);
- int ret;
- ret = dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0);
- /* drm_gem_mmap_obj() has already been called before this function
* and has incremented our refcount, expecting it to be decremented* on unmap() via drm_gem_vm_close().* However dma_buf_mmap() invokes drm_gem_cma_prime_mmap()* that ends up updating the vma->vma_private_data to point to the* dma_buf's gem object.* Hence our gem object here will not have its refcount decremented* when userspace does unmap().* So decrement the refcount here to avoid a memory leak if the dma* buf mapping was successful.*/- if (!ret)
drm_gem_object_put_unlocked(&etnaviv_obj->base);- return ret;
} static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
Hi Lucas,
Am Mittwoch, den 20.05.2020, 12:10 +0200 schrieb Martin Fuzzey: What's the use-case where you did hit this issue? mmap'ing of imported buffers through the etnaviv DRM device is currently not well defined and I was pondering the idea of forbidding it completely by not returning a mmap offset for those objects.
I hit this on Android 8 (on i.MX6 using mesa 20.0.6 with gbm gralloc and drm hwcomposer) and had a memory leak every time an activity was started. I'm not sure exactly why but Android does a gralloc.lock() and gralloc.unlock() on every activity startup. Those map and unmap the buffer.
Under Android (at least in 8+) the actual graphics buffer allocations are done by a dedicated process (android.hardware.graphics.allocator@2.0-service) because it uses a "binderized HAL" for the allocation [https://source.android.com/devices/architecture/hal-types] This means that buffers are *always* imported (though they are usually only mmaped for SW rendering or screen shots).
Regards,
Martin
Hi
[This is an automated email]
This commit has been processed because it contains a -stable tag. The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v5.6.13, v5.4.41, v4.19.123, v4.14.180, v4.9.223, v4.4.223.
v5.6.13: Build OK! v5.4.41: Build OK! v4.19.123: Build OK! v4.14.180: Build OK! v4.9.223: Build OK! v4.4.223: Failed to apply! Possible dependencies: 0e7f26e6b950 ("drm/etnaviv: take etnaviv_gem_obj in etnaviv_gem_mmap_obj") 9f07bb0d4ada ("drm/etnaviv: fix get pages error path in etnaviv_gem_vaddr") a0a5ab3e99b8 ("drm/etnaviv: call correct function when trying to vmap a DMABUF") a10e2bde5d91 ("drm/etnaviv: fix mmap operations for userptr and dma-buf objects") a8c21a5451d8 ("drm/etnaviv: add initial etnaviv DRM driver")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
linux-stable-mirror@lists.linaro.org
-
Fuzzey, Martin -
Lucas Stach -
Martin Fuzzey
-
Sasha Levin