[Intel-gfx] [PATCH] drm/i915: fix one mem leak in mmap_offset_attach()

[Tvrtko Ursulin]

On 23/02/2022 00:42, Chuansheng Liu wrote:
> The below memory leak information is caught:
> 
> ===
> unreferenced object 0xffff997dd4e3b240 (size 64):
>    comm "gem_tiled_fence", pid 10332, jiffies 4294959326 (age
> 220778.420s)
>    hex dump (first 32 bytes):
>      01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>      00 00 00 00 00 00 00 00 00 be f2 d4 7d 99 ff ff  ............}...
>    backtrace:
>      [<ffffffffa0f04365>] kmem_cache_alloc_trace+0x2e5/0x450
>      [<ffffffffc062f3ac>] drm_vma_node_allow+0x2c/0xe0 [drm]
>      [<ffffffffc13149ea>] __assign_mmap_offset_handle+0x1da/0x4a0 [i915]
>      [<ffffffffc1315235>] i915_gem_mmap_offset_ioctl+0x55/0xb0 [i915]
>      [<ffffffffc06207e4>] drm_ioctl_kernel+0xb4/0x140 [drm]
>      [<ffffffffc0620ac7>] drm_ioctl+0x257/0x410 [drm]
>      [<ffffffffa0f553ae>] __x64_sys_ioctl+0x8e/0xc0
>      [<ffffffffa1821128>] do_syscall_64+0x38/0xc0
> [<ffffffffa1a0007c>] entry_SYSCALL_64_after_hwframe+0x44/0xae
> ===
> 
> The issue is always reproduced with the test:
> gem_tiled_fence_blits --run-subtest basic
> 
> It tries to mmap_gtt the same object several times, it is like:
> create BO
> mmap_gtt BO
> unmap BO
> mmap_gtt BO <== second time mmap_gtt
> unmap
> 
> The leak happens at the second time mmap_gtt in function
> mmap_offset_attach(),it will simply increase the reference
> count to 2 by calling drm_vma_node_allow() directly since
> the mmo has been created at the first time.
> 
> However the driver just revokes the vma_node only one time
> when closing the object, it leads to memory leak easily.
> 
> This patch is to fix the memory leak by calling drm_vma_node_allow() one
> time also.

Fix looks correct to me after a brief analysis. Matt or Thomas, could you please spare a 2nd pair of eyes on this?

Alternative could be to add drm_vma_node_revoke_all, which would drop all references to the node, which would perhaps be more in the spirit of the ref counting scheme used inside i915_gem_mman.c, but it would not be desirable as a fix which needs backporting. So I think this patch is the way to go and maybe tweak later, once minimal fix propagates to upstream.

> Cc: abdiel.janulgue at linux.intel.com
> Cc: matthew.auld at intel.com
> Cc: chris at chris-wilson.co.uk
> Signed-off-by: Chuansheng Liu <chuansheng.liu at intel.com>

Fixes: 786555987207 ("drm/i915/gem: Store mmap_offsets in an rbtree rather than a plain list")
Cc: <stable at vger.kernel.org> # v5.7+

Regards,

Tvrtko

> ---
>   drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> index efe69d6b86f4..d50b2f643a10 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> @@ -680,7 +680,7 @@ mmap_offset_attach(struct drm_i915_gem_object *obj,
>   	mmo = insert_mmo(obj, mmo);
>   	GEM_BUG_ON(lookup_mmo(obj, mmap_type) != mmo);
>   out:
> -	if (file)
> +	if (file && !drm_vma_node_is_allowed(&mmo->vma_node, file))
>   		drm_vma_node_allow(&mmo->vma_node, file);
>   	return mmo;
>