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@xxxxxxxxxx <mailto:srinivas.kandagatla@xxxxxxxxxx>>
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@xxxxxxxxxxxxxx
<mailto:mak.chopra@xxxxxxxxxxxxxx>>
Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@xxxxxxxxxx
<mailto:jorge.ramirez-ortiz@xxxxxxxxxx>>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx
<mailto:srinivas.kandagatla@xxxxxxxxxx>>
---
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
