On 28/08/2019 08:50, Jorge Ramirez wrote:
On 8/27/19 23:45, Srinivas Kandagatla wrote:
On 23/08/2019 16:23, Jorge Ramirez-Ortiz wrote:
can you add me as a co-author to this patch please?
No problem I can do that if you feel so!
yes please. thanks!
since I spent about a day doing the analysis, sent you a fix that
maintained the API used by the library and explained you how to
reproduce the issue I think it is just fair. > the fact that the api
could be be modified and the fix be done a bit
differently- free dma buf ioctl removed- seems just a minor
implementation detail to me.
No, that's not true, this is a clear fastrpc design issue.
IMO the ioctls defines the contract with userspace and the contract
establishes that userspace must call deallocate. the kernel wrongly
implemented to that contract since it doesn't handle the cases where
userspace can't send the release calls which leads to memory leaks. this
is what I meant by and implementation issue.
if we had many fastrpc users, rolling out the design change that you
propose - removing an ioctl- would definitively have an impact. But
since that is not yet the case, there is not doubt that your patch makes
more sense.
Exactly before it make a way into other projects!
but my point was that there is not a huge gap in efforts between doing
one or the other.
Thats not the point, point is about right fix!
Userspace is already doing a refcount via mmap/unmap on that dmabuf fd,
having an additional api adds another level of refcount which is totally
redundant and is the root cause for this leak.
yes it is redundant but is not the root cause for this leak. the root
cause is that the driver doesnt handle the case where userspace didnt or
was not able to call release (and that is no more than adding allocated
buffers to a list and clean on exit)
I don't agree with you on that. We should not take an extra refcount in
first place in driver.
let me explain it one more time!
dmabuf has to be mmaped in userspace app before it is used, and
"Memory mappings that were created in the process shall be unmapped
before the process is destroyed" so the refcount is taken care by
mmap/unmap automatically.
--srini
