lkml.org 
[lkml]   [2019]   [Aug]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 4/5] misc: fastrpc: fix double refcounting on dmabuf
Date

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
>

\
 
 \ /
  Last update: 2019-08-27 23:46    [W:0.062 / U:2.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site