Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@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: Tue, 27 Aug 2019 22:45:46 +0100
Message-ID: <cb003c11-d331-a2c7-1eb4-a89ba025f4c6@linaro.org> (raw)
In-Reply-To: <CAC8LzUAnz+RZYh+bBbJbXJYP3QDq4H1847W8rJxj-aF1B1J9QQ@mail.gmail.com>


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!

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

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.


--srini

> 
> also you can add my tested-by if you want
> 
> TIA
> 
> On Fri, 23 Aug 2019 at 12:07, Srinivas Kandagatla 
> <srinivas.kandagatla@linaro.org <mailto:srinivas.kandagatla@linaro.org>> 
> wrote:
> 
>     dma buf refcount has to be done by the driver which is going to use
>     the fd.
>     This driver already does refcount on the dmabuf fd if its actively
>     using it
>     but also does an additional refcounting via extra ioctl.
>     This additional refcount can lead to memory leak in cases where the
>     applications fail to call the ioctl to decrement the refcount.
> 
>     So remove this extra refcount in the ioctl
> 
>     More info of dma buf usage at drivers/dma-buf/dma-buf.c
> 
>     Reported-by: Mayank Chopra <mak.chopra@codeaurora.org
>     <mailto:mak.chopra@codeaurora.org>>
>     Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org
>     <mailto:jorge.ramirez-ortiz@linaro.org>>
>     Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org
>     <mailto:srinivas.kandagatla@linaro.org>>
>     ---
>       drivers/misc/fastrpc.c | 25 -------------------------
>       1 file changed, 25 deletions(-)
> 
>     diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
>     index 38829fa74f28..eee2bb398947 100644
>     --- a/drivers/misc/fastrpc.c
>     +++ b/drivers/misc/fastrpc.c
>     @@ -1198,26 +1198,6 @@ static int fastrpc_device_open(struct inode
>     *inode, struct file *filp)
>              return 0;
>       }
> 
>     -static int fastrpc_dmabuf_free(struct fastrpc_user *fl, char __user
>     *argp)
>     -{
>     -       struct dma_buf *buf;
>     -       int info;
>     -
>     -       if (copy_from_user(&info, argp, sizeof(info)))
>     -               return -EFAULT;
>     -
>     -       buf = dma_buf_get(info);
>     -       if (IS_ERR_OR_NULL(buf))
>     -               return -EINVAL;
>     -       /*
>     -        * one for the last get and other for the ALLOC_DMA_BUFF ioctl
>     -        */
>     -       dma_buf_put(buf);
>     -       dma_buf_put(buf);
>     -
>     -       return 0;
>     -}
>     -
>       static int fastrpc_dmabuf_alloc(struct fastrpc_user *fl, char
>     __user *argp)
>       {
>              struct fastrpc_alloc_dma_buf bp;
>     @@ -1253,8 +1233,6 @@ static int fastrpc_dmabuf_alloc(struct
>     fastrpc_user *fl, char __user *argp)
>                      return -EFAULT;
>              }
> 
>     -       get_dma_buf(buf->dmabuf);
>     -
>              return 0;
>       }
> 
>     @@ -1322,9 +1300,6 @@ static long fastrpc_device_ioctl(struct file
>     *file, unsigned int cmd,
>              case FASTRPC_IOCTL_INIT_CREATE:
>                      err = fastrpc_init_create_process(fl, argp);
>                      break;
>     -       case FASTRPC_IOCTL_FREE_DMA_BUFF:
>     -               err = fastrpc_dmabuf_free(fl, argp);
>     -               break;
>              case FASTRPC_IOCTL_ALLOC_DMA_BUFF:
>                      err = fastrpc_dmabuf_alloc(fl, argp);
>                      break;
>     -- 
>     2.21.0
> 

  parent 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 [this message]
2019-08-28  7:50       ` Jorge Ramirez
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=cb003c11-d331-a2c7-1eb4-a89ba025f4c6@linaro.org \
    --to=srinivas.kandagatla@linaro.org \
    --cc=arnd@arndb.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=jorge.ramirez-ortiz@linaro.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mak.chopra@codeaurora.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 linux-arm-msm@archiver.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