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

[김승우]

On 2012년 11월 20일 19:26, Maarten Lankhorst wrote:
> Op 20-11-12 02:03, 김승우 schreef:
>> Hi Maarten,
>>
>> On 2012년 11월 19일 19:27, Maarten Lankhorst wrote:
>>> 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 at samsung.com>
>>>> Signed-off-by: Kyungmin.park <kyungmin.park at samsung.com>
>>>> Cc: Inki Dae <inki.dae at samsung.com>
>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>> Cc: Rob Clark <rob.clark at linaro.org>
>>>> Cc: Alex Deucher <alexander.deucher at 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 believe my patch can fix all issue related with gem prime
>> completely. But it seems that it can solve unrecoverable memory leak
>> caused by re-importing GEM. Anyway, can you give me an example of other
>> race issue?
> When the dma_buf is unreffed and refcount drops to 0, work is queued to free it.
> 
> Until then export_dma_buf member points to something, but refcount is 0 on it.
> 
> Importing to itself, then closing fd then re-export from the new place would probably trigger it.
> 

Ok, I understood about issue from delayed fput also in your below
comment. Anyway, IMO, it is already on current drm prime.

>>> 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.
>> I cannot figure out all aspect of delayed fput, but until delayed work
>> is called, dma_buf is not released so export_dma_buf is valid.
>> Considering this, I can't find possible issue of delayed fput.
> I'm fairly sure that when refcount drops to 0, even though the memory may not be freed yet,
> you no longer have a guarantee that the memory is still valid.

I missed that delayed fput is triggered after recount drops to 0, and
now I understood it can cause invalid access.

> 
> And of course after importing into a different process with its own drm namespace, how does
> file_priv->prime.lock still protect serialization?
> 
> I don't see any hierarchy either. export_dma_buf needs to be set to NULL before the dma_buf_put
> is called that is used for the reference inside export_dma_buf.
> 
> The release function should only release a reference to whatever backing memory is used.
> 
>>> 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.
>> Considering re-import, where drm file priv is different from exporter's
>> file priv, attach and detach are not called because gem object is reused.
>>
>> How do you think remove checking whether dma_buf is came from driver own
>> exporter? Because without this checking, gem object is newly created,
>> and then re-import issue will disappear.
> The whole refcounting is a circular mess, sadly with no easy solution. :/
> 
> It seems to me that using gem reference counts is solving this problem at the wrong layer.
> A secondary type of reference count is needed for the underlying memory backing.
> 
> For those who use ttm, I would recommend that the dma_buf increases refcount on ttm_bo by one,
> so that the gem object can be destroyed and release its reference on the dma_buf.
> 
> WIthout a secondary refcount you end up in a position where you can't reliably and race free unref
> the gem object and dma_buf at the same time, since they're both independent interfaces with possibly
> different lifetimes.

If there is no checking whether dma_buf is came from driver own
exporter, gem_object is newly allocated and refcount of it can be a
secondary refcount at least form mermoy leak issue. So I mentioned about
removing check for exporter.
But I prefer processing re-import as gem_open without considering
dma-buf as current implementation.

> 
> It would really help if there were hard rules on lifetime of the export_dma_buf member,
> until then we're just patching one broken thing with another.

Issue about memory leak of re-import was also reported on i915 and there
was Rob's patch set, but other locking issue blocked the patch.
I'm not sure all issue can be solved at once and someone is handling
this at the moment.

Best Regards,
- Seung-Woo Kim

> 
> 
> ~Maarten
> 
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--