Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Jorge Ramirez <jorge.ramirez-ortiz@linaro.org>
To: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Greg KH <gregkh@linuxfoundation.org>,
	arnd@arndb.de, linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Mayank Chopra <mak.chopra@codeaurora.org>
Subject: Re: [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf
Date: Wed, 28 Aug 2019 09:50:46 +0200
Message-ID: <d545f7fc-fc97-c250-b9d2-ebfbc9709780@linaro.org> (raw)
In-Reply-To: <cb003c11-d331-a2c7-1eb4-a89ba025f4c6@linaro.org>

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.

but my point was that there is not a huge gap in efforts between doing
one or the other.

> 
> 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)

> 
> 
> --srini


  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-23 10:06 [PATCH 0/5] misc: fastrpc: few fixes Srinivas Kandagatla
2019-08-23 10:06 ` [PATCH 1/5] misc: fastrpc: Reference count channel context Srinivas Kandagatla
2019-08-28 20:53   ` Greg KH
2019-08-23 10:06 ` [PATCH 2/5] misc: fastrpc: Don't reference rpmsg_device after remove Srinivas Kandagatla
2019-08-28 15:41   ` Stephen Boyd
2019-08-23 10:06 ` [PATCH 3/5] misc: fastrpc: remove unused definition Srinivas Kandagatla
2019-08-23 10:06 ` [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf Srinivas Kandagatla
     [not found]   ` <CAC8LzUAnz+RZYh+bBbJbXJYP3QDq4H1847W8rJxj-aF1B1J9QQ@mail.gmail.com>
2019-08-27 21:45     ` Srinivas Kandagatla
2019-08-28  7:50       ` Jorge Ramirez [this message]
2019-08-28  8:48         ` Srinivas Kandagatla
2019-08-28 10:35           ` Jorge Ramirez
2019-08-23 10:06 ` [PATCH 5/5] misc: fastrpc: free dma buf scatter list Srinivas Kandagatla

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d545f7fc-fc97-c250-b9d2-ebfbc9709780@linaro.org \
    --to=jorge.ramirez-ortiz@linaro.org \
    --cc=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mak.chopra@codeaurora.org \
    --cc=srinivas.kandagatla@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git