Re: [RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

Op 15-11-12 04:52, Seung-Woo Kim schreef:
> Increasing ref counts of both dma-buf and gem for imported dma-buf
> come from gem makes memory leak. release function of dma-buf cannot
> be called because f_count of dma-buf increased by importing gem and
> gem ref count cannot be decrease because of exported dma-buf.
>
> So I add dma_buf_put() for imported gem come from its own gem into
> each drivers having prime_import and prime_export capabilities. With
> this, only gem ref count is increased if importing gem exported from
> gem of same driver.
>
> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> Signed-off-by: Kyungmin.park <kyungmin.park@samsung.com>
> Cc: Inki Dae <inki.dae@samsung.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Rob Clark <rob.clark@linaro.org>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> ---
> Mmap failiure in i915 was reported by Jani, and I think it was fixed
> with Dave's commit "drm/prime: add exported buffers to current fprivs
> imported buffer list (v2)", so I resend this patch.
>
> I can send exynos only, but this issue is common in all drm driver
> having both prime inport and export, IMHO, it's better to send for
> all drivers.
I fear that  this won't solve the issue completely and keeps open a small race.

I don't like the current destruction path either. It really looks like export_dma_buf
should be unset when the exported buffer is closed in the original file. Anything else is racy.
Especially setting export_dma_buf to NULL won't work with the current delayed fput semantics.

So to me it seems instead we should do something like this:

- dmabuf_release callback is a noop, no ref is held by the dma-buf.
- attach and detach ops increase reference by 1*.

- when the buffer is exported, export_dma_buf is set by core code and kept around
  alive until the gem object is closed.

- dma_buf_put is not called by import function. This reference removed as part
  of gem object close instead.

~Maarten

*) Lockdep will rightfully hate this as it reopens a locking inversion, some solution for that is needed.

Maybe a post detach callback for dma-buf with all locks dropped would be best here?
Other way would be to schedule delayed work with the sole purpose of unreffing in the detach op,
similar to how atomic_dec_and_mutex_lock works.